Capture the exception at the catch site, not inside the library - #87
Merged
Merged
Conversation
An exception thrown from a task scheduled with scheduleBulk is lost on Windows clang shared-library builds, surfacing to the caller as std::bad_exception instead of the thrown type. TaskSetBase::trySetCurrentException() called std::current_exception() in its own body, which is compiled into the dispenso library, while every caller's catch (...) is header code compiled into the caller's module. Under clang on Windows, std::current_exception() called from a DLL for an exception thrown in the executable returns null. The null exception_ptr was stored, and testAndResetException() then called std::rethrow_exception(nullptr) -- undefined behaviour, which the MSVC runtime converts into std::bad_exception. Only scheduleBulk surfaced it because non-bulk tasks run on pool threads created inside the library, where the library's std::current_exception() works. The bulk path can run tasks inline on the caller's thread via invokeInline, which is exactly the configuration that returns null. The four call sites now pass std::current_exception() in and the library only stores it. testAndResetException() also stops rethrowing a null pointer, so a future capture failure reports a cancelled set rather than a meaningless bad_exception. Also moves #include <exception> in detail/pipeline_impl.h out of that file's #if __cplusplus >= 201703L block: std::current_exception() is used there under a plain __cpp_exceptions guard, so a C++14 build with exceptions enabled needs the header unconditionally. It resolves today only through a transitive include from task_set.h.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #90.
An exception thrown from a task scheduled with
scheduleBulkdoes not reach thecaller on Windows clang shared-library builds. Instead of the thrown type, the
caller sees
std::bad_exceptionout ofwait().task_set_shared_testreproduces it deterministically — 10 failures in 10 runsof
TaskSet.ScheduleBulkException. The static build passes, and MSVC passes.Cause
TaskSetBase::trySetCurrentException()callsstd::current_exception()in itsown body. That body is compiled into the dispenso library, while every caller's
catch (...)is header code compiled into the caller's module.Under clang on Windows,
std::current_exception()called from a DLL, for anexception thrown in the executable, returns null. The null
exception_ptris stored, and
testAndResetException()later callsstd::rethrow_exception(nullptr)— undefined behaviour, which the MSVC runtimeconverts into
std::bad_exception. Nothing reports the original loss, so thefailure surfaces far from its cause.
Reduced to a minimal reproduction with no threads and no dispenso — throw in the
executable, call
std::current_exception()inside a library module:Why only the bulk path
Non-bulk tasks run on pool threads created inside the library, where the
library's
std::current_exception()works. The bulk path can run tasks inlineon the caller's thread via
invokeInline— the executable's thread — which isexactly the configuration that returns null.
Fix
Capture where the exception is live: the four call sites pass
std::current_exception()in, and the library only stores it.testAndResetException()additionally stops rethrowing a null pointer, so ifcapture ever fails again the set is reported as cancelled rather than
manufacturing a
bad_exception.This changes the signature of
TaskSetBase::trySetCurrentException, which isDISPENSO_DLL_ACCESS. It is adetailmember and the header ships with thelibrary, but it is an ABI change for anyone mixing versions.
The patch also moves
#include <exception>indetail/pipeline_impl.hout ofthat file's
#if __cplusplus >= 201703Lblock:std::current_exception()isused there under a plain
__cpp_exceptionsguard, so a C++14 build withexceptions needs the header unconditionally. It currently resolves only through
a transitive include from
task_set.h.Results with the fix, on Windows:
task_set_shared_testtask_set_test(static)future_shared_testtask_set_shared_testLinux, full suite: 1564 passed, 0 failed.
Note on cost
Capture now happens at every catch site rather than only for the winner of the
CAS, so concurrent throwers each construct an
exception_ptr— an atomicrefcount increment under the Itanium ABI, an object copy under MSVC, on a path
that has just paid for a throw and an unwind. The non-throwing path is
untouched.
Suggested follow-up test
No existing test would have caught this outside a Windows shared build. A test
asserting that the thrown type survives — rather than merely that something
was thrown — from a
scheduleBulktask would.