背景
之前一个应用有一个小功能,每隔1分钟创建一个线程来清理无效的http连接
主要逻辑之前是这样写的
private volatile boolean running = true;
@Override
public void run() {
try {
while (running) {
synchronized (this) {
wait(1000);
closeConnections();
}
}
} catch (InterruptedException ex) {
shutdown();
}
}
public void shutdown() {
running= false;
synchronized (this) {
notifyAll();
}
}
问题说明
最终的影响就是 不停地创建线程 直到线程溢出 OOM!
分析
有经验的同学应该能看到这里有个running,这个就是为了标识线程要退出的信号量
但是 用错了位置,或者说用错了时间,导致线程一直是running状态 而无法获得notify退出
解决办法
private volatile boolean running = true;
@Override
public void run() {
try {
while (running ) {
synchronized (this) {
wait(1000);
running= false;
connMgr.closeExpiredConnections();
connMgr.closeIdleConnections(5 * 60, TimeUnit.SECONDS);
logger.trace("clear closed http connections ok ,next to exit");
}
}
} catch (InterruptedException ex) {
shutdown();
}
}
public void shutdown() {
// running= false;need to mark at wait
synchronized (this) {
notifyAll();
}
}
效果
线程能正常被notify 然后退出
拓展
其实这样写也有不合理的地方 比如try while的范围,一般来讲 synchronized是写在while外面的,而 try 是写在 while里面的,这样也符合此场景下功能要求,当然这样写也不能说有啥大问题,总体是正能量的
另外 也不需要用wait, 因为就这一个线程在处理,可以sleep再直接退出 不涉及竞争的问题。