[SerialExecutor] SerialExecutor.checkIsolated() to check its own tracking for isolation checks #71172
Conversation
ktoso
force-pushed
the
wip-concurrency-assert-dispatch
branch
from
d513606 to
ce4abce
Compare
ktoso
requested review from
chimz,
rjmccall,
DougGregor,
rokhinip and
PriyaAvhad
ktoso
added
the
concurrency
| let executor: NaiveQueueExecutor | ||
|
|
||
| init() { | ||
| self.queue = DispatchSerialQueue(label: "MyQueue") |
There was a problem hiding this comment.
Can we add a similar test for DispatchWorkloop being used as serial executor?
There was a problem hiding this comment.
Sure, I can give that a go.
It's a bit unfortunate that I have to keep writing "wrappers" since the conformances don't ship in old enough OS or on Linux, so such "wrapper executor around X" is the best I can do, rather than directly using the queue or workloop...
but I'll give that a go.
There was a problem hiding this comment.
TODO: I've not done this bit yet but will follow up on it
|
@swift-ci please smoke test |
ktoso
changed the title
ktoso
force-pushed
the
wip-concurrency-assert-dispatch
branch
from
f65fafb to
b263491
Compare
ktoso
force-pushed
the
wip-concurrency-assert-dispatch
branch
from
b263491 to
63cf486
Compare
|
@swift-ci please build toolchain |
| return false; | ||
| } | ||
|
|
||
| if (expectedExecutor.isMainExecutor() && !currentExecutor.isMainExecutor()) { |
There was a problem hiding this comment.
Also here - We need to consider dispatch serial queue targeting the main queue, right?
There was a problem hiding this comment.
Looping back to this, sorry to keep you waiting.
Agreed in pricinple, but we should carefully roll out this change -- if we did this now, we would get much worse error messages. The current message includes "expected MainActor executor" which the default implementation that'd SerialDispatchQueue would bene up using would not have.
We need dispatch to implement these APIs with good error messages "you expected main but was something else" I think?
If we think this doesn't matter I could already make this call checkIsolated though, but I think we should stagger the rollout ^
ktoso
force-pushed
the
wip-concurrency-assert-dispatch
branch
2 times, most recently
from
b88f14e to
ba6fa6c
Compare
ktoso
changed the title
ktoso
force-pushed
the
wip-concurrency-assert-dispatch
branch
3 times, most recently
from
17d1afe to
ded2187
Compare
|
@swift-ci please test |
|
@swift-ci please test |
ktoso
force-pushed
the
wip-concurrency-assert-dispatch
branch
from
6a7a8b8 to
f86cd1b
Compare
|
Failure was a missing |
ktoso
force-pushed
the
wip-concurrency-assert-dispatch
branch
from
f86cd1b to
84891f8
Compare
|
@swift-ci please smoke test |
|
hmm real failure specific to x86 macos |
Allow serial executors to perform a "last resort" check before crashing in calls like preconditionIsolated and assumeIsolated. This allows custom excecutors to use external information to claim that while the synchronous code is not executing in a Swift Task, it IS executing on the apropriate thread or queue the code is expecting. add swift_task_checkIsolatedImpl to CooperativeGlobalExecutor
ktoso
force-pushed
the
wip-concurrency-assert-dispatch
branch
from
df6e286 to
7608717
Compare
| /// an application was linked to. Since Swift 6 the default is to crash, | ||
| /// and the logging behavior is no longer available. | ||
| static unsigned unexpectedExecutorLogLevel = | ||
| swift_bincompat_useLegacyNonCrashingExecutorChecks() |
There was a problem hiding this comment.
This specifically handles "app compiled against old sdk should still be able to get the warnings"
fyi @xedin
| #else | ||
| return false; // Always use the new behavior on non-Apple OSes | ||
| #endif | ||
| } |
There was a problem hiding this comment.
The dyld compatibility trickery would go here once we have a release
|
@swift-ci please smoke test |
|
@swift-ci please smoke test |
|
@swift-ci please smoke test |
|
@swift-ci please smoke test Linux |
…king for isolation checks (swiftlang#71172)
This implements the proposal SE-0424
resolves rdar://112637467
Related thread: https://forums.swift.org/t/dispatchqueue-serialexecutor-isolation-check-is-not-consistent-on-darwin/69389/4
In addition, we need to be more careful when upgrading the executor check to "default mode is crashing", and should only do so in the upcoming release.
This adds a runtime check that we'll replace with checking what version of the SDKs an app was linked against. If it is the release with Swift 6 (future), we'll make it crash by default, but by doing it dynamically like this -- old apps, linked against ols SDKs will be allowed to keep working, until they rebuild with a new SDK and get the new behavior.
The changes related to that semi-undo the flag swap from: 8787846
And prepare the "hook" we'll have to implement once the the releases are announced in the future in Bincompat.cpp.
Resolves rdar://125107864