Protect WaitingTask*Holder::taskHasFailed from a data race #38765
Conversation
The call to WaitingTaskHolder::taskHasFailed could attempt to call exceptionPtr while it is being set. The function is now protected from a data race.
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38765/31092
|
|
A new Pull Request was created by @Dr15Jones (Chris Jones) for master. It involves the following packages:
@cmsbuild, @smuzaffar, @Dr15Jones, @makortel can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
| ///Called if waited for task failed | ||
| /**Allows transfer of the exception caused by the dependent task to be | ||
| * moved to another thread. | ||
| * This method should only be called by WaitingTaskList | ||
| */ | ||
| void dependentTaskFailed(std::exception_ptr iPtr) { | ||
| bool isSet = false; | ||
| if (iPtr and m_ptrSet.compare_exchange_strong(isSet, true)) { | ||
| unsigned char isSet = static_cast<unsigned int>(State::kUnset); |
There was a problem hiding this comment.
In general a nice solution to the overall issue. Are the static_cast's necessary? isSet and the enum are both already unsigned char.
There was a problem hiding this comment.
@wddgit it wouldn't compile without the static_casts.
There was a problem hiding this comment.
I think the explicit cast is needed because of enum class (with plain enum cast should not be necessary). But why not cast via unsigned char as both source and destination are only that wide?
|
please test |
|
-1 Failed Tests: UnitTests Unit TestsI found errors in the following unit tests: ---> test TestSubProcess had ERRORS Comparison SummarySummary:
|
The function now returns an std::exception_ptr by value not by pointer anymore.
Dr15Jones
force-pushed
the
protect_taskHasFailed
branch
from
7a502ff
to
58e901e
Compare
cmsbuild
added
code-checks-pending
tests-pending
and removed
tests-rejected
code-checks-approved
labels
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38765/31093
|
|
Pull request #38765 was updated. @cmsbuild, @smuzaffar, @Dr15Jones, @makortel can you please check and sign again. |
|
Please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-32d402/26287/summary.html Comparison SummarySummary:
|
| }; | ||
|
|
||
| template <typename F> | ||
| class FunctorWaitingTask : public WaitingTask { | ||
| public: | ||
| explicit FunctorWaitingTask(F f) : func_(std::move(f)) {} | ||
|
|
||
| void execute() final { func_(exceptionPtr() ? &exceptionPtr() : nullptr); }; | ||
| void execute() final { func_(uncheckedExceptionPtr() ? &uncheckedExceptionPtr() : nullptr); }; |
There was a problem hiding this comment.
Mainly for future reference, the uncheckedExceptionPtr() can be used here because this function will be called only after the reference count has reached 0, and therefore nothing else can be writing into the m_ptr at the same time, right?
|
+1 |
cmsbuild
added
core-approved
fully-signed
and removed
core-pending
pending-signatures
labels
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
PR description:
WaitingTask*Holder::taskHasFailed could call exceptionPtr() while the underlying object is being set. This change makes calls to exceptionPtr() safe across threads.
Thanks to @wddgit for catching this case.
PR validation:
Code compiles and FWCore/Concurrency unit tests pass.