HTTPServer Applications Slow to Terminate #3796

[sbalousek]

• Queue a StopNotification for each thread in the TCPServerDispatcher thread pool. Otherwise, only one thread in the thread pool is terminated.

Signed-off-by: Stephen Balousek sbalousek@wickedloop.com

[Burgch]

Is there a reason this couldn't use NotificationQueue::wakeupAll() instead?

[aleks-f]

@Burgch I think you are right, this change does not guarantee that all threads will receive the notification; also, with wakeUpAll(), the StopNotification, and _queue.clear() becomes unnecessary here.

@obiltschnig ?

[Burgch]

I was going to create a pull request based on diff, but I don't think it fully solves the problem. The issue is that there could always be threads that are in the loop after the end of the lock scope but not yet started waiting in the call to _queue.waitDequeueNotification().

I think the existing code (this PR) is actually correct, because it ensures there are at least as many notifications as there could be threads which are currently waiting, or about to start waiting. I think using _currentThreads instead of _threadPool.allocated() for the size of the loop might make more sense, as it could be an external threadpool which is doing lots of other things too.

[obiltschnig]

Poco::NotificationQueue::wakeupAll() will only wake up threads that are currently in waitDequeueNotification(), so it's never sufficient to reach all threads working on the queue. Enqueueing a StopNotification for each thread is the correct solution.

[Burgch]

Agreed - I think the only change I'd now suggest is to switch to queuing only _currentThreads StopNotifications.

[aleks-f]

@Burgch _currentThreads is more optimal, but an additional barrier in stop() and enqueue() is needed to prevent new requests from being enqueued after stop.

diff --git a/Net/src/TCPServerDispatcher.cpp b/Net/src/TCPServerDispatcher.cpp
index 2d974ae93..05d1840fb 100644
--- a/Net/src/TCPServerDispatcher.cpp
+++ b/Net/src/TCPServerDispatcher.cpp
@@ -140,11 +140,18 @@ namespace
 
 void TCPServerDispatcher::enqueue(const StreamSocket& socket)
 {
+       if (_stopped)
+       {
+               ++_refusedConnections;
+               return;
+       }
+
        FastMutex::ScopedLock lock(_mutex);
 
        if (_queue.size() < _pParams->getMaxQueued())
        {
                _queue.enqueueNotification(new TCPConnectionNotification(socket));
+
                if (!_queue.hasIdleThreads() && _currentThreads < _pParams->getMaxThreads())
                {
                        try
@@ -172,8 +179,9 @@ void TCPServerDispatcher::enqueue(const StreamSocket& socket)
 
 void TCPServerDispatcher::stop()
 {
+       if (_stopped.exchange(true)) return;
+
        FastMutex::ScopedLock lock(_mutex);
-       _stopped = true;
        _queue.clear();
        for (int i = 0; i < _threadPool.allocated(); i++)
        {

[Burgch]

@aleks-f I don't believe that is necessary, thanks to TCPServer::stop(), which stops and joins the polling thread (which is the only place that calls enqueue) before it stops the TCPServerDispatcher:

void TCPServer::stop()
{
	if (!_stopped)
	{
		_stopped = true;
		_thread.join();
		_pDispatcher->stop();
	}
}