@alex65536 wouldn't just adding check for _stopped fix the problem?

if (_pDisp->_stopped || (_pDisp->_currentThreads > 1 && _pDisp->_queue.empty()))

@alex65536 wouldn't just adding check for _stopped fix the problem?

I don't think so. The bug is that the server is slowly degrading, and the server doesn't need to be stopped in order to reproduce it. So I don't believe that just adding stopped_ will help.

@alex65536 wouldn't just adding check for _stopped fix the problem?

I don't think so. The bug is that the server is slowly degrading, and the server doesn't need to be stopped in order to reproduce it. So I don't believe that just adding stopped_ will help.

OK, but then the fix you submitted won't fix it either, because it's doing the same thing as the destructor would if modified as I suggested. If I'm missing something, please explain in detail, "slowly degrading" does not make sense to me; I admit I have scarce time to spend on this, but I do not want to again merge fix which is not fixing the problem.

The difference is that both the current code and your proposed change perform the checks twice, and both checks don't happen under the same lock. This PR will perform the check only once.

I was not proposing to check it twice. I meant to (1) modify the check in the ~ThreadCountWatcher() and (2) remove the one at the bottom of TCPServerDispatcher::run() for loop. I just didn't explicitly stated the (2), but that's why I said it was the same thing. but I guess it doesn't really matter which way it is done since it is the same thing in the end.

Any progress on the review?

Just wanted to say thank you for this fix - it also fixes a race condition where the value of _currentThreads can be decremented excessively. It's basically the opposite of the race condition described in ClickHouse#28 - if the queue is empty when the ThreadCountWatcher is destructed, but then not empty when the condition is checked at the end of the for-loop, the thread will continue and construct a new ThreadCountWatcher on the next iteration. This means _currentThreads can be decremented more than once by any given thread.