Skip to content

[Executors] Improved executor precondition errors #64171

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

[Executors] Improved executor precondition errors #64171

wants to merge 2 commits into from

Conversation

@ktoso
Copy link
Contributor

@ktoso ktoso commented Mar 7, 2023 •
edited
Loading

Looking how getting it just right is rather tricky, i.e. the PR over here just now: #64169 I'm in inclined to try to get the same semantics as closely as possible, by reusing precondition most probably

This improves the executor precondition error messages from:

before:
Precondition failed: Incorrect actor executor assumption; Expected 'UnownedSerialExecutor(executor: (Opaque Value))' executor.

improved:
Precondition failed: Incorrect actor executor assumption; Expected 'MainActorExecutor' executor, but was executing on 'a.InlineExecutor'.

which is also part of the reason to offer precondition/assert/assume APIs that do the crashing for us, instead of giving users a "get executor" and have them write precondition() themselfes.

resolves / relates to rdar://106188692

@ktoso ktoso requested review from DougGregor, kavon, mikeash and rjmccall and removed request for mikeash March 7, 2023 13:14
ktoso
ktoso commented Mar 7, 2023
View reviewed changes
ktoso
ktoso commented Mar 7, 2023
View reviewed changes
ktoso
ktoso commented Mar 7, 2023
View reviewed changes
ktoso
ktoso commented Mar 7, 2023
View reviewed changes
ktoso
ktoso commented Mar 7, 2023
View reviewed changes
ktoso
ktoso commented Mar 7, 2023
View reviewed changes
ktoso
ktoso commented Mar 7, 2023
View reviewed changes
@ktoso ktoso requested a review from jckarter March 8, 2023 02:31
@ktoso ktoso self-assigned this Mar 8, 2023
@ktoso ktoso added the concurrency Feature: umbrella label for concurrency language features label Mar 8, 2023
@ktoso ktoso changed the title RFC: improved executor precondition errors Improved executor precondition errors Mar 8, 2023
@ktoso ktoso changed the title Improved executor precondition errors [Executors] Improved executor precondition errors Mar 8, 2023
@ktoso ktoso force-pushed the wip-improved-precondition-errors branch 2 times, most recently from 86c8217 to 3d637d1 Compare March 8, 2023 13:54
@ktoso
Copy link
Contributor Author

ktoso commented Mar 8, 2023

@swift-ci please smoke test

ktoso
ktoso commented Mar 8, 2023
View reviewed changes
test/Concurrency/Runtime/actor_assume_executor_unknown.swift

if #available(SwiftStdlib 5.9, *) {
tests.test("assumeOnMainActorExecutor: wrongly assume the main executor, from DispatchQueue") {
expectCrashLater(withMessage: "Expected same executor as actor 'Swift.MainActor' ('MainActorExecutor'), but was executing on '<unknown>'.")
Copy link
Contributor Author

@ktoso ktoso Mar 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The unknown here is very much on purpose - we're not on a "swift concurrency executor" at this point in time.

@ktoso
Copy link
Contributor Author

ktoso commented Mar 9, 2023

Need to verify on optimized build, something is slighly off there

@ktoso ktoso mentioned this pull request Mar 10, 2023
Merged
@ktoso ktoso force-pushed the wip-improved-precondition-errors branch from 02ac432 to 86a90c8 Compare March 10, 2023 05:35
@ktoso ktoso force-pushed the wip-improved-precondition-errors branch from 86a90c8 to a804cb5 Compare March 10, 2023 05:38
@ktoso
Copy link
Contributor Author

ktoso commented Mar 10, 2023

@swift-ci please smoke test

@ktoso
Copy link
Contributor Author

ktoso commented Mar 10, 2023

So... ../build/Ninja-ReleaseAssert does pass locally, and it seems it is only the optimized build on linux that is failing. Need to get a linux build to reproduce this 🤔

@ktoso
Copy link
Contributor Author

ktoso commented Mar 10, 2023

@swift-ci please smoke test

mac weird crash: sh -c '/Applications/Xcode-beta.app/Contents/Developer/usr/bin/xcodebuild -sdk /Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk -find dyld_info 2> /dev/null' failed with exit code 17664: (null) (errno=Invalid argument)

@ktoso
Copy link
Contributor Author

ktoso commented Mar 10, 2023

@swift-ci please smoke test

@ktoso
Copy link
Contributor Author

ktoso commented Mar 15, 2023

@swift-ci please smoke test macOS

@ktoso ktoso force-pushed the wip-improved-precondition-errors branch from a804cb5 to ee506b3 Compare March 15, 2023 03:32
@ktoso
Copy link
Contributor Author

ktoso commented Mar 15, 2023

@swift-ci please smoke test

ktoso added 2 commits March 15, 2023 14:41
@ktoso
[Executors] improved executor precondition messages
268c10c 
document the get active current executor method
@ktoso
[Executors] Specialized message for failed assert between 2 default a…
9aab398 
…ctors

fix missing default initializer for ExecutorActiveAndRef
@ktoso ktoso force-pushed the wip-improved-precondition-errors branch from b3dc571 to 9aab398 Compare March 15, 2023 05:41
@ktoso
Copy link
Contributor Author

ktoso commented Mar 15, 2023

@swift-ci please smoke test

@xedin xedin removed their request for review March 30, 2023 23:19
@ktoso ktoso closed this Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DougGregor DougGregor Awaiting requested review from DougGregor

@rjmccall rjmccall Awaiting requested review from rjmccall rjmccall is a code owner

@kavon kavon Awaiting requested review from kavon

@mikeash mikeash Awaiting requested review from mikeash

@jckarter jckarter Awaiting requested review from jckarter

@slavapestov slavapestov Awaiting requested review from slavapestov

@hborla hborla Awaiting requested review from hborla

Assignees

@ktoso ktoso

Labels

concurrency Feature: umbrella label for concurrency language features

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@ktoso