Threadpool deadlocks during shutdown on Windows

[debaetsd]

When using C exit function, the IlmThreadpool deadlocks during shutdown on Windows.

This is happening because the worker threads have not run yet (and so have not posted on the "started" semaphore) when they get killed.
During shutdown, that semaphore deadlocks as it waits for N threads (but it might not have been posted on all workers)
This is especially noticeable when oversubscribing threads (for example 10 threads on a 2 core machine).

I have never seen it deadlock when calling the exit function from the main thread (though not sure if that means it cannot happen or just random coinsidence) and Windows seems to be the only platform affected.

This was initially discovered when trying to get the OpenImageIO test suite to run on Windows yet the following code can be used to reproduce (you might have to run it a couple times because, well threading :) )

int main(int argc, char** argv)
{
	std::thread t([]() {exit(-1);	});
	Imf::setGlobalThreadCount(5);
	t.join();
}

[meshula]

exit() is defined to terminate an entire process, including child threads, and transfer control to the host environment. Exit does a certain amount of cleanup that is largely implementation specific. exit also doesn't terminate instantaneously, but causes termination at "some time" in the future. A likely fatal execution order is

main stdlib startup
initialize t, queue lambda for execution
run lambda
invoke exit
clean up stdlib and other execution state
leave thread
stdlib already cleaned up, thread library in undefined state
Imf::setGlobalThreadCount executes mutex
sadness

Are you using exit(-1) in the thread to force the deadlock you see, or is there actually an exit involved in your test scenario?

[debaetsd]

Some of the tools inside OIIO do call exit upon certain failures (and the tests invoke those tools).
Besides the fact that OIIO should strive for a cleaner error strategy, it can be an issue.

The sample above is just a repro case to trigger it and gives the following callstack

	IlmThread-2_5.dll!IlmThread_::Semaphore::wait() Line 110
	IlmThread-2_5.dll!IlmThread_::anonymous namespace'::DefaultThreadPoolProvider::finish() Line 428
 	IlmThread-2_5.dll!IlmThread_::ThreadPool::Data::~Data() Line 569	
	IlmThread-2_5.dll!IlmThread_::ThreadPool::~ThreadPool() Line 738
 	[External Code]	
 	Test01.exe!main::__l2::<lambda>() Line 98

again, I'm not entirely sure the exit should be invoked from within that additional thread, it is just the easiest to repro that way.

I toyed a bit with it and testing if the worker threads are alive inside DefaultThreadPoolProvider::finish (and if not, do not wait on the semaphore) seem to able to fix it though not sure if that is the correct solution.

[meshula]

Can you try this?

Right now there's this code in IlmThreadPool.cpp:

    size_t curT = _data.threads.size();
    for (size_t i = 0; i != curT; ++i)
    {
        _data.taskSemaphore.post();
        _data.threadSemaphore.wait();
    }

    //
    // Join all the threads
    //
    for (size_t i = 0; i != curT; ++i)
    {
        _data.threads[i]->join();
        delete _data.threads[i];
    }

instead:

    size_t curT = _data.threads.size();
    for (size_t i = 0; i != curT; ++i)
    {
        if (_data.threads[i]->joinable()) {
            _data.taskSemaphore.post();
           _data.threadSemaphore.wait();
       }
    }

    //
    // Join all the threads
    //
    for (size_t i = 0; i != curT; ++i)
    {
        if (_data.threads[I]->joinable()) {
            _data.threads[i]->join();
        }
        delete _data.threads[i];
    }

[debaetsd]

I'll give it a try

[cary-ilm]

@kdt3rd, will your core rewrite address this?

[lgritz]

This is biting my CI badly right now (only on Windows) and I'm struggling to find a workaround. WBN to fix for 3.0 (and any future 2.5 we may have as well).

[meshula]

@lgritz Are you able to check if my patch helps?

[lgritz]

Um... I don't have a Windows machine, but I can try to rig up my CI to pull and build OpenEXR from your branch and see if that helps. Will try to get to that over the weekend.

[meshula]

@lgritz, I haven't pushed the proposed patch to my fork... I'd like to see this sorted out as well, so I can try it in the next couple of days if no one beats me to it.

[debaetsd]

Sorry for not getting back earlier about this.
I did a quick test with that patch (https://github.com/debaetsd/openexr/tree/winthread) and the dummy repro case from above seems to work now.
Testing it inside oiio atm

[meshula]

Ah, thanks very much, I appreciate it :)

[debaetsd]

So it turned out that this patch in itself was not fixing the issue :(

In the end, I did found a solution (more in line with my initial thought of "check if the threads are still alive").
This is however a Windows specific solution atm as I don't think it can be done with just using std::thread (though as this seems Windows only issue, that might not be an issue?)
This makes the OIIO windows CI pass (well tbf, not exactly pass but not deadlock, see https://github.com/debaetsd/oiio/runs/1714550750. required a bit of
whac-a-mole code changes as it seems oiio + OpenExr master does not compile on windows).

So now the question remains what the way forward is.
I could clean up my changes (master...debaetsd:winthread) though somebody here would need to validate them.
Both on a "is it ok to have some Windows specific stuff in here" level as on a higher "is this not breaking the expected behavior of the ThreadPool class" level.

Let me know how you wish to proceed with this

[lgritz]

I'm still mystified about why this is popping up now, fairly reliably for you and on the Windows CI on GH, but none of the Windows users of OIIO have previously reported this problem (nor any users of OpenEXR, to the best of my knowledge). I guess as far as OpenEXR goes, a change like this is probably the right idea (though I don't know enough about Windows threads to properly review this), and perhaps in addition to having the fix in 3.0, we should backport to one last 2.5 patch. But it does bug me that it means we can't build and test OIIO against OpenEXR <= 2.4 on Windows, I wish there was some kind of (even awkward) OIIO-side "workaround" that would work even for older OpenEXR and/or without requiring the patch in OpenEXR itself.

[debaetsd]

tbh I do not know why it suddenly started being an issue, I have been using OpenEXR for years and never seen it up until the point I started looking at the OIIO CI. Given that this is somewhat in the c++ undefined lands, it might even be something that the Windows OS pseudo-recently changed