Page MenuHomePhabricator

[Support][Parallel] ThreadPoolExecutor fixes for Windows
AbandonedPublic

Authored by andrewng on Nov 22 2017, 10:35 AM.

Details

Reviewers
zturner
Bigcheese
Summary
  • Disable use of MS ConcRT as it can cause crashes on exit, particularly with the MSVC static debug CRT.
  • Changed ThreadPoolExecutor to not use detached threads but instead to join threads where possible on destruction. This is to prevent random crashing on Windows when doing a full exit, e.g. via exit().
  • Changed ThreadPoolExecutor to be a ManagedStatic so that it can be shutdown on llvm_shutdown(). Otherwise this could only occur when doing a full exit, e.g. via exit(). This is required to avoid a race condition on Windows between the ThreadPoolExecutor still starting up threads and the process doing a fast exit, e.g. via _exit().

Diff Detail

Event Timeline

andrewng created this revision.Nov 22 2017, 10:35 AM
zturner requested changes to this revision.Nov 22 2017, 10:46 AM
zturner added a reviewer: Bigcheese.
zturner added a subscriber: zturner.

I absolutely do not want to disable support for ConcRT under any circumstances unless we find that it is a completely broken library, which I highly doubt. Even then, I would want to ask Microsoft to fix it.

What is the exact problem you're running into?

This revision now requires changes to proceed.Nov 22 2017, 10:46 AM

I absolutely do not want to disable support for ConcRT under any circumstances unless we find that it is a completely broken library, which I highly doubt. Even then, I would want to ask Microsoft to fix it.

What is the exact problem you're running into?

Hi, sorry, I thought that this would be a "private" patch. Don't worry, I do not intend to make any changes without proper review!

I created this patch to see if it might help with a problem being seen on Linux, in the message thread "std::thread::detach() rarely crashes in Parallel.cpp".

The reason I've been looking into this is as mentioned in the summary. The MS Concurrency Run-time can cause exceptions at exit, particularly when used with the MSVC static debug CRT, which in turn causes LLD to intermittently crash on exit. We have already submitted a bug to Microsoft, but no progress has been made. We also discovered whilst researching the issue, that the MSVC STL used to use the MS ConcRT but no longer does...

Locally, we've been running with MS ConcRT disabled, as we need to use the MSVC static CRT's.

I absolutely do not want to disable support for ConcRT under any circumstances unless we find that it is a completely broken library, which I highly doubt. Even then, I would want to ask Microsoft to fix it.

What is the exact problem you're running into?

Hi, sorry, I thought that this would be a "private" patch. Don't worry, I do not intend to make any changes without proper review!

Ahh, sorry about that. One of the Phabricator triggers must have added me automatically.

I created this patch to see if it might help with a problem being seen on Linux, in the message thread "std::thread::detach() rarely crashes in Parallel.cpp".

The reason I've been looking into this is as mentioned in the summary. The MS Concurrency Run-time can cause exceptions at exit, particularly when used with the MSVC static debug CRT, which in turn causes LLD to intermittently crash on exit. We have already submitted a bug to Microsoft, but no progress has been made.

Can you link me to the connect issue?

We also discovered whilst researching the issue, that the MSVC STL used to use the MS ConcRT but no longer does...

At least on my local MSVC 2017 installation, it's still used in <future> and <mutex>. We don't have an implementation of Parallel TS yet from Microsoft, but I would imagine that's going to use it behind the scenes. If their plan is to deprecate it, they're definitely being quiet about it :)

labath added a subscriber: labath.Nov 28 2017, 12:55 AM

I absolutely do not want to disable support for ConcRT under any circumstances unless we find that it is a completely broken library, which I highly doubt. Even then, I would want to ask Microsoft to fix it.

What is the exact problem you're running into?

Hi, sorry, I thought that this would be a "private" patch. Don't worry, I do not intend to make any changes without proper review!

I created this patch to see if it might help with a problem being seen on Linux, in the message thread "std::thread::detach() rarely crashes in Parallel.cpp".

I have no idea about windows, but for the linux issue, you should look into whether you're not running into https://sourceware.org/bugzilla/show_bug.cgi?id=19951. We've ran into that in LLDB, where we also spawn a ton of detached threads. If you can send me some core dumps or steps to reproduce I may be able to help you with diagnosing that. If this is indeed the same bug it is fairly easy to work around it without ditching detached threads. If you can send me a core dump or steps to reproduce I may be able to help you with diagnosing that.

andrewng abandoned this revision.Tue, Nov 19, 8:39 AM

Latest version of this patch is now in D70447.