[Concurrency] Avoid inserting handler record in already cancelled task. #80456
Conversation
This avoids the potential to race with the triggering coming from task_cancel, because we first set the cancelled flag, and only THEN take the lock and iterate over the inserted records. Because of this we could: T1 flip the cancelled bit; T2 observes that, and triggers "immediately" during installing the handler record. T1 then proceeds to lock records and trigger it again, causing a double trigger of the cancellation handler. resolves swiftlang#80161 resolves rdar://147493150
|
@swift-ci please test |
|
@swift-ci please smoke test |
| return; | ||
| } | ||
|
|
||
| if (auto poppedRecord = |
There was a problem hiding this comment.
The way the remove and add are paired is such that the record should always be at the top here, so this removal should be exactly that record on the top.
There was a problem hiding this comment.
Would be good to add an assert here that task->_private()._status().load(std::memory_order_relaxed).getInnermostRecord() == record to check that.
|
Failure was TestSwiftActorUnprioritisedJobs.py which seems to be flaky, see also swiftlang/llvm-project#10404 trying to fix it |
|
@swift-ci please smoke test macOS |
| /// an explicit instantiation in TaskStatus.cpp. | ||
| template <typename TaskStatusRecordT> | ||
| SWIFT_CC(swift) | ||
| TaskStatusRecordT* popStatusRecordOfType(AsyncTask *task); |
There was a problem hiding this comment.
We may want to move the templated parts of the implementation into the header to avoid annoying link issues.
There was a problem hiding this comment.
I did try that but we don’t have the types fully declared there hmmm, I can try again. I’ll share the error
There was a problem hiding this comment.
It's fine to leave it if C++ objects too strenuously to my idea.
There was a problem hiding this comment.
Yeah it's a bit painful, we'd need complete type declarations for ActiveTaskStatus which is somewhat annoying to pull up completely.
/Users/ktoso/code/swift-project/swift/stdlib/public/Concurrency/TaskPrivate.h:255:27: warning: extra qualification on member 'popStatusRecordOfType' [-Wextra-qualification]
255 | TaskStatusRecordT* swift::popStatusRecordOfType(AsyncTask *task) {
| ^
/Users/ktoso/code/swift-project/swift/stdlib/public/Concurrency/TaskPrivate.h:255:27: error: out-of-line definition of 'popStatusRecordOfType' does not match any declaration in namespace 'swift'
255 | TaskStatusRecordT* swift::popStatusRecordOfType(AsyncTask *task) {
| ^~~~~~~~~~~~~~~~~~~~~
/Users/ktoso/code/swift-project/swift/stdlib/public/Concurrency/TaskPrivate.h:258:54: error: variable has incomplete type 'ActiveTaskStatus'
258 | removeStatusRecordWhere(task, [&](ActiveTaskStatus s, TaskStatusRecord *r) {
| ^
/Users/ktoso/code/swift-project/swift/stdlib/public/Concurrency/TaskPrivate.h:58:7: note: forward declaration of 'swift::ActiveTaskStatus'
58 | class ActiveTaskStatus;
| ^
I'll leave as is I think but thank you very much for review!
There was a problem hiding this comment.
We could have a small templatized implementation in the header that just retrieves the kind of the type being requested, then calls into a real non-templatized implementation which looks for that kind. But we can save that for when we actually need it.
This avoids the potential to race with the triggering coming from task_cancel, because we first set the cancelled flag, and only THEN take the lock and iterate over the inserted records. Because of this we could: T1 flip the cancelled bit; T2 observes that, and triggers "immediately" during installing the handler record. T1 then proceeds to lock records and trigger it again, causing a double trigger of the cancellation handler.
resolves #80161
resolves rdar://147493150