Why did my thread pool stop processing work once it hit a long-running work item?


A customer found that occasionally, their program's thread pool stopped processing work items queued with the WT_EXECUTE­IN­PERSISTEN­THREAD flag. They would queue up the work items to the thread pool, but the work items would not get dispatched. Naturally, this caused problems with the program because certain background actions would simply stall.

After some investigation, the problem was traced to this work item that appears to be preventing the thread pool from processing any work:

ntdll!NtWaitForMultipleObjects+0xc
KERNELBASE!WaitForMultipleObjectsEx+0xcc
kernel32!WaitForMultipleObjects+0x19
contoso!WidgetMonitor::WidgetNotificationCallback+0xfd
contoso!std::tr1::_Callable_fun<void (__stdcall*const)
    (std::tr1::shared_ptr<WidgetService>),0>::_ApplyX+0x1b
contoso!std::tr1::_Impl_no_alloc1<std::tr1::_Callable_fun<
    void (__stdcall*const)(std::tr1::shared_ptr<WidgetService>),0>,
    void,std::tr1::shared_ptr<WidgetService> &>::_Do_call+0x2d
contoso!std::tr1::_Function_impl1<void,std::tr1::shared_ptr<
    WidgetMonitor::WidgetNotificationContext> &>::operator()+0x1e
contoso!Win32Adapters::Threading::Callback<std::tr1::shared_ptr<
    WidgetMonitor::WidgetNotificationContext> >::
    ExecuteCallbackTarget+0x3f
contoso!Win32Adapters::Threading::Callback<std::tr1::shared_ptr<
    WidgetMonitor::WidgetNotificationContext> >::
    DefaultThreadProc+0xd
ntdll!RtlpTpWorkCallback+0xef
ntdll!TppWorkerThread+0x4f3
kernel32!BaseThreadInitThunk+0x24
ntdll!__RtlUserThreadStart+0x2f
ntdll!_RtlUserThreadStart+0x1b

(I inserted line breaks for readability.)

Once they closed the widget monitor, the thread pool woke up and the work items that targeted the persistent thread started running again.

Okay, first things first: For expository purposes, let's remove all of the std::tr1 stuff and pretend that the stack was this:

ntdll!NtWaitForMultipleObjects+0xc
KERNELBASE!WaitForMultipleObjectsEx+0xcc
kernel32!WaitForMultipleObjects+0x19
contoso!WidgetMonitor::WidgetNotificationCallback+0xfd
ntdll!RtlpTpWorkCallback+0xef
ntdll!TppWorkerThread+0x4f3
kernel32!BaseThreadInitThunk+0x24
ntdll!__RtlUserThreadStart+0x2f
ntdll!_RtlUserThreadStart+0x1b

That gets rid of the project's internal callback scaffolding and lets us focus on the interaction with the operating system.

The problem isn't really visible in the stack trace. We'll have to go to the code.

void WidgetMonitor::WidgetNotificationCallback(void* parameter)
{
 WidgetNotificationContext* context =
   reinterpret_cast<WidgetNotificationContext*>(parameter);

 RAII_HKEY hkey = ...;
 RAII_HANDLE registryEvent = ...;
 bool keepWaiting = true;
 while (keepWaiting) {
  if (RegNotifyChangeKeyValue(hkey, false, REG_NOTIFY_CHANGE_LAST_SET,
                              registryEvent, TRUE) == ERROR_SUCCESS) {
   HANDLE handles[2] = { registryEvent, context->shutdownEvent };
   DWORD waitResult = WaitForMultipleObjects(2, handles, FALSE, INFINITE);
   switch (waitResult) {
   case WAIT_OBJECT_0: // the registry key changed
    ...
    break;
   case WAIT_OBJECT_0+1: // we are being asked to shut down
    ...
    keepWaiting = false;
    break;
   default: // Something unexpected happened
    ...
    keepWaiting = false;
    break;
   }
  }
 }
}

The deal is that the callback function processes the callback, and then goes into a loop monitoring a registry key. It continues monitoring the key until the shutdown event is signaled.

Okay, so this looks a little weird, holding a thread pool thread hostage for an extended period of time, which is sort of contrary to the intent of a thread pool, which is to reuse a thread for multiple short work items. But it's technically legal, and you are encouraged to pass the WT_EXECUTE­LONG­FUNCTION flag to tell the thread pool, "This function will take a long time, so you may want to schedule work onto other threads more aggressively instead of sitting around waiting for this work item to finish."

But the problem is that the program didn't pass only the WT_EXECUTE­LONG­FUNCTION flag. It did this:

BOOL WidgetMonitor::StartMonitoringChangeNotifications()
{
  WidgetNotificationContext context = ...;
  return QueueUserWorkItem(
    WidgetMonitor::WidgetNotificationCallback,
    context, WT_EXECUTELONGFUNCTION | WT_EXECUTEINPERSISTENTTHREAD);
}

Notice that they requested that the callback run in the persistent thread. But the documentation for that flag says

This flag should be used only for short tasks…

So we have a contradiction. One flag says, "Run this callback in a persistent thread, and I promise I don't take a long time." The other flag says, "I'm going to take a long time."

The original thread pool was a bit too trusting and assumed that nobody would be so crazy as to explicitly declare their intent to break the rules.¹ I mean, if you're going to break the rules, you are probably going to be sneaky about it, right? It so happened that the way the thread pool code was written, the WT_EXECUTE­IN­PERSISTEN­THREAD flag takes precedence. The callback runs in the persistent thread, even though it runs long.²

And that's why the thread pool persistent thread grinds to a halt. The persistent thread is running the callback function, and the callback function is stuck. As a result, the persistent thread can't do anything else, and the thread pool makes no progress. This also explains why shutting down widget notifications caused everything to wake up: Shutting down widget notifications causes the Widget­Config::Widget­Notification­Callback function to break out of its loop and finally exit. This releases the persistent thread to run more work items.

Okay, so we've diagnosed the problem. Next time, we'll speculate as to why the developers chose to combine contradictory threads and (perhaps more important) suggest a solution.

¹ Actually, what's happening is that the two flags are targeting different parts of the thread pool. The "persistent thread" flag is an instruction to the thread pool work item dispatcher, telling it to dispatch the work item to a persistent thread. The "long function" flag is an instruction to the thread pool throughput manager to let it know that it should prefer to start a new thread instead of waiting for the work item to complete. Neither component on its own noticed anything wrong.

² If we had a time machine, we could go back and make this combination of flags cause Queue­User­Work­Item fail with ERROR_INVALID_PARAMETER, but unfortunately that option is not available to us. We're stuck with the existing behavior of allowing the contradictory flags and ignoring the WT_EXECUTE­LONG­FUNCTION flag.

Comments (15)
  1. David Haim says:

    I always assumed the win32 thread pool uses some sort of hill-climbing with the number of threads to prevent exactly this kind of problems, isn’t it?

    1. Not sure how hill-climbing comes into play. The deal is that “the persistent thread” is the thread that dispatches the work to worker threads. For very short tasks, you can run them directly in the persistent thread. This means they dispatch faster, but it also means that nothing else gets dispatched until you’re done.

      1. Joshua says:

        In which case I can imagine somebody doing this on purpose to pause the entire pool.

      2. David Haim says:

        Yes, but is it mandatory that there’s only one singular persistence thread? The threadpool can see after sometime that the persistence thread is not free, add yet another persistence thread, see if the throughput of the workitems completion rate went up (it should in this case), stop for few seconds, and retry again.

        1. So you’re saying that the thread pool should work around other people’s programming errors? The “don’t need to be compatible with apps that break the rules” people might disagree with you on that.

          1. David Haim says:

            Well, I have to say that MSDN is really not clear on this, MSDN only writes “. This flag should be used only for short tasks or it could affect other timer operations.” , so for me, as a developer, that implies that if my thread pool doesn’t consume any timers it’s not the end of the world. MSDN does not really say that this is the dispatcher thread, and you also assume that as a thread pool, it knows to handle with long tasks (and you can clearly see in your debugger that the thread pool injects more threads if needed, aka the hill climbing), you assume it’s not the end of the world if your task takes longer. I wouldn’t call it “programming error” as much as I call it “ambiguous documentation”

        2. I feel like that would increase the complexity of the threadpool considerably to support a workflow that the threadpool wasn’t designed to support (at which point the appropriate response would be, “write your own threadpool or grab one of the many libraries that does this for you”). Honestly I’m surprised the WT_EXECUTEINPERSISTENTTHREAD flag exists. I feel like that is just begging for abuse and kind of defeats the purpose of having a threadpool.

  2. alegr1 says:

    “We need a persistent thread to monitor registry changes”
    “Why not run a workitem with WT_EXECUTE­IN­PERSISTEN­THREAD flag then?”
    !!!!Profit!!!!

  3. SimonRev says:

    My guess is that the devs didn’t read MSDN, and whoever named the flag WT_EXECUTE­IN­PERSISTEN­THREAD was wearing kernel-colored-glasses. From a developer PoV, the name WT_EXECUTE­IN­PERSISTEN­THREAD kind of sounds like you want the thread pool to allocate a permanent thread to you. So they were helpfully trying to tell the thread pool that this thread would be running a long time and so issue it its own thread.

  4. Wear says:

    Wouldn’t it be nice if all API issues could be solved with a time machine?

    “It hurts when I do this”
    “I have just made it so that you were never able to do that”
    “Do what?”

  5. mikeb says:

    > The deal is that “the persistent thread” is the thread that dispatches the work to worker threads. For very short tasks, you can run them directly in the persistent thread.

    To put it simply: if an app uses the WT_EXECUTEINPERSISTENTTHREAD but it turns out to be a long running work item, then the whole thread pool is stalled for as long as that work item is being handled. It doesn’t matter whether the WT_EXECUTELONGFUNCTION is specified or not.

    > We’re stuck with the existing behavior of allowing the contradictory flags and ignoring the WT_EXECUTELONGFUNCTION flag.

    If you wanted to be nice to apps that pass the contradictory flags, couldn’t a change be made so that the WT_EXECUTELONGFUNCTION flag takes precedence? That way the dispatcher would spin up a new thread for the work item (or pass it to an existing thread that isn’t the dispatcher thread). This would leave the dispatcher free to handle new work items. The cost would be that if the thread actually was a short task it might not be handled as efficiently. But it would still be handled.

    I don’t see a downside to such a change, but maybe I’m missing something (like something to do with APCs). Of course, the developer can still shoot themselves in the foot if they lie and use only the WT_EXECUTEINPERSISTENTTHREAD flag for a long running task. But at least some small subset of apps that attempt to tell the truth by also setting the WT_EXECUTELONGFUNCTION would be helped.

    1. Harry Johnston says:

      Existing code that sometimes used both flags and sometimes used only WT_EXECUTE­IN­PERSISTEN­THREAD might break when work items that were expected to run from the same thread suddenly start running on two different threads. (Granted, the code was already broken, but possibly only in the sense of running inefficiently; your proposed change could mean it no longer worked at all.)

      It might be safe for the thread pool to create a dedicated “persistent thread” separate from the dispatcher thread. But that would defeat the original purpose of the flag, and make properly written code less efficient than it should be.

      1. Ismo says:

        Where was it implied that the persistent thread would be the very same every time ? As a developer I would have assumed that the are some persistent threads put a side which are used when WT_EXECUTEINPERSISTENTTHREAD was specified. The documentation is not good here.

        1. Harry Johnston says:

          Actually, now that I check, the documentation explicitly says that it ISN’T guaranteed to be the same thread each time.
          Nonetheless, if that is the current implementation, there is likely to be existing code that depends on it. :-(

      2. mikeb says:

        It’s been a while since I’ve had to deal with this kind of stuff on Windows, but in general combining a thread pool with something that ‘has to run on a particular thread’ seems like a bad idea to me. To me a threadpool should be used for work that can occur on any thread. If the work needs to happen on a particular thread it shouldn’t be done using a threadpool (or the work done on the threadpool should just pass the real work on to the correct thread). But maybe I’m over simplifying real world issues.

Comments are closed.

Skip to main content