httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Evgeny Kotkov <evgeny.kot...@visualsvn.com>
Subject [RFC/PATCH] mpm_winnt: Fix several issues in the child process shutdown logic
Date Fri, 07 Jul 2017 10:18:43 GMT
Hi all,

Recently we (Ivan Zhakov and I) found several issues in the code responsible
for shutting down a mpm_winnt's child process.

The attached patch should fix these issues:

 (Please note that the changes are combined into a single patch to make the
  review easier, but I'll commit them independently.)

 (1) As a part of the process of shutting down worker threads, the code
     around child.c:1170 currently posts an amount of I/O completion packets
     equal to the amount of the threads blocked on the I/O completion port.
     Then it sleeps until all these threads "acknowledge" the completion
     packets by decrementing the global amount of blocked threads.

     This can be improved to avoid unnecessary Sleep()'s, and make the
     shutdown faster.  There is no need to block until the threads actually
     receive the completion, as the shutdown process includes a separate step
     that waits until the threads exit.  Instead of synchronizing on the
     amount of the threads blocked on the I/O completion port, we can send
     the number of IOCP_SHUTDOWN completion packets equal to the total
     amount of threads and immediately proceed to the next step.

  (2) If the shutdown routine hits a timeout while waiting for the worker
      threads to exit, it uses TerminateThread() to terminate the remaining
      threads.

      Using TerminateThread() can have dangerous consequences such as
      deadlocks — say, if the the thread is terminated while holding a lock
      or a heap lock in the middle of HeapAlloc(), as these locks would not
      be released.  Or it can corrupt the application state and cause a crash.
      See https://msdn.microsoft.com/en-us/library/windows/desktop/ms686717

      Presumably, a much better alternative here would be to leave the cleanup
      to the operating system by calling TerminateProcess().

  (3) Assuming (2) is in place, the code around child.c:1170 that waits for
      multiple thread handles in batches can be simplified.  With (2), there
      is no difference between ending the wait with one or multiple remaining
      threads.  (Since we terminate the process if at least one thread is
      still active when we hit a timeout.)

      Therefore, instead of making an effort to evenly distribute and batch
      the handles with WaitForMultipleObjects(), we could just start from
      one end, and wait for one thread handle at a time.

Note that apart from this, the code around child.c:1146 that shuts down the
listeners, waits, and then proceeds to shutting down the worker threads is
prone to a subtle race.  Since the wait interval is hardcoded to 1 second,
there could still be an accepted connection after it.  And, as the worker
threads are being shut down, it is feasible that this accepted connection
won't ever find a suitable worker thread.  (Eventually, it is going to be
ungracefully closed).  I think that this can be fixed by properly waiting for
the listener threads to exit, and in the meanwhile, this would avoid having
the Sleep(1000) call altogether.  But this is something that I have now left
for future work.

I would greatly appreciate if someone could review or comment on the proposed
changes.  If anyone has an additional insight on the design or planned, but
unaccomplished changes to the mpm_winnt module, I would appreciate hearing
them as well.

Thanks!


Regards,
Evgeny Kotkov

Mime
View raw message