Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Making crossplat::threadpool::shared_instance() safe to use in cpprest DLL #611

Merged

Conversation

garethsb
Copy link
Contributor

This took me a while to track down. While investigating PR #608, on Windows / VS 2013, I found that many of the test suites would more often than not complete successfully, but then the test_runner exe wouldn't exit. I found this was due to deadlock while the test_module_loader was unloading DLLs. The culprit is the destruction of the static threadpool_impl inside crossplat::threadpool::shared_instance(). Its destructor must join all the threads in the pool. This is likely to deadlock because in DllMain at DLL_PROCESS_DETACH, the loader lock is held, and the thread to be joined therefore cannot signal thread termination by DLL_THREAD_DETACH. (A quick web search finds many explanations of this, including some good ones on Microsoft's own The Old New Thing.)

Why hasn't this been reported before? I assume mostly on Windows, people aren't using e.g. CPPREST_FORCE_HTTP_LISTENER_ASIO or if they are, they aren't explicitly unloading the cpprest DLL until process termination.

There are two approaches to fixing this:

  1. Don't expose a shared instance via the current API. A reference-counted interface, e.g. exposing shared_ptr<threadpool>, could work.
  2. Keep the API. Jump through hoops internally to ensure we can't deadlock in the destruction of the static instance.

Assuming 2 is preferable, I ultimately discovered that asio already provides the mechanism to solve this exact issue. And it has been present and documented in asio since at least Boost 1.37.0, so although it is in the namespace boost::asio::detail I think it can be relied on. I have tried Boost 1.65.1 (current) and Boost 1.54.0 (cpprestsdk's documented minimum version). I have run tests on both Windows and Linux platforms.

Implementation notes:

  • I'd have kept the #if defined(CPPREST_PTHREADS) alternative, but boost::asio::detail::thread picks Windows threads or POSIX threads (or falls back to std::thread) appropriately anyway, so the code is now mostly simpler than it was too.
  • I'd have used std::vector<boost::asio::detail::thread>, but for some reason the thread type isn't movable, hence the use of std::unique_ptr.

…re the process exits, the crossplat::threadpool::shared_instance() is destroyed at DLL_PROCESS_DETACH, at which stage joining threads causes deadlock. Use the workaround provided by asio to terminate the threads in this case.
…e thread may be holding e.g. the heap lock, so we need to ensure the thread is in a known state. Rather than reinvent the wheel, use asio's own thread class which already provides the necessary mechanism.
@msftclas
Copy link

msftclas commented Nov 27, 2017

CLA assistant check
All CLA requirements met.

…functionality (e.g. complete_after) that needs access to the io_service when using the asio-based default scheduler
@ras0219-msft ras0219-msft merged commit eba38b8 into microsoft:master Jan 24, 2018
@ras0219-msft
Copy link
Contributor

Thank you for the PR and the detailed writeup! I have to concede as well that using boost::asio::detail::thread is the current optimal solution here. I do wish we could reduce dependency on boost over time, but it doesn't look like the case here :)

I've removed the restriction to only follow this process in DLLs, because it's unfortunately common for people to link things built as static libs into DLLs. It shouldn't harm EXEs either, so it seems clear that we should just always use this new path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants