Skip to content
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.

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

Revert "Revert "Isolated synchronous deinit"" #76250

Merged
merged 10 commits into from
Oct 3, 2024

Conversation

ktoso
Copy link
Contributor

@ktoso ktoso commented Sep 4, 2024

Remove task-local tests from isolated deinit, as we don't guarantee behavior of them

This was changing back and forth during review but proposal now states that

This proposal does not define how Swift runtime should behave when
running isolated deinit. It may use task-local values as seen at the
point of the last release, reset them to default values, or use some
other set of values. Behavior is allowed to change without notice. But
future proposals may change specification by defining a specific
behavior.

and indeed we observed different behaviors failing the test. It seems we may want to drop that test entirely tbh.

@ktoso
Copy link
Contributor Author

ktoso commented Sep 4, 2024

@swift-ci please test

@benrimmington
Copy link
Contributor

The proposal hasn't been updated yet, but it was accepted with modifications:

Lastly, on the topic of task locals, the LSG agreed with reviewers who expressed that the approach of 'make no promises' was likely to result in users inadvertently relying on implementation details which would turn out to be difficult to change later, and so decided to adopt the consistent rule that task locals will always be cleared when entering an isolated deinit.

@ktoso
Copy link
Contributor Author

ktoso commented Sep 4, 2024

Heh, my bad for assuming the proposal is up to date. Thanks for the reminder

@ktoso ktoso force-pushed the wip-experimental-isolated-deinit branch from eb11a01 to eecb20d Compare September 5, 2024 08:46
@benrimmington
Copy link
Contributor

I've created swiftlang/swift-evolution#2558 to update the proposal.

@ktoso
Copy link
Contributor Author

ktoso commented Sep 5, 2024

Thanks, LGTM on that update.

The implementation does not match that yet actually I just remembered (and checked)...

@ktoso
Copy link
Contributor Author

ktoso commented Sep 5, 2024

Still trying to figure out how the failures here could have ever happened... they're not possible to reproduce reliably heh.

@ktoso
Copy link
Contributor Author

ktoso commented Sep 5, 2024

A crash we saw is

Command Output (stdout):
--
[ RUN      ] Isolated Deinit.class sync fast path
[       OK ] Isolated Deinit.class sync fast path
[ RUN      ] Isolated Deinit.class sync slow path
stdout>>> check failed at /Users/ec2-user/jenkins/workspace/oss-swift_tools-RA_stdlib-RD_test-simulator/swift/test/Concurrency/Runtime/async_task_locals_isolated_deinit.swift, line 121
stdout>>> first:  105553152229376 (of type Swift.Int)
stdout>>> second: 0 (of type Swift.Int)
stderr>>> CRASHED: SIGILL
the test crashed unexpectedly
[     FAIL ] Isolated Deinit.class sync slow path
[ RUN      ] Isolated Deinit.actor sync fast path
[       OK ] Isolated Deinit.actor sync fast path
[ RUN      ] Isolated Deinit.actor sync slow path
[       OK ] Isolated Deinit.actor sync slow path
[ RUN      ] Isolated Deinit.no TLs
[       OK ] Isolated Deinit.no TLs
Isolated Deinit: Some tests failed, aborting
UXPASS: []
FAIL: ["class sync slow path"]
SKIP: []
To debug, run:
$ /Users/ec2-user/jenkins/workspace/oss-swift_tools-RA_stdlib-RD_test-simulator/build/Ninja-ReleaseAssert/swift-macosx-x86_64/test-macosx-x86_64/Concurrency/Runtime/Output/async_task_locals_isolated_deinit.swift.tmp/a.out --stdlib-unittest-in-process --stdlib-unittest-filter "class sync slow path"

showing a corrupted value on self -- the task local value is properly 0 as expected though...

@ktoso
Copy link
Contributor Author

ktoso commented Sep 5, 2024

@swift-ci please test

@ktoso ktoso force-pushed the wip-experimental-isolated-deinit branch from eecb20d to 1b1d9de Compare September 18, 2024 03:50
@ktoso ktoso force-pushed the wip-experimental-isolated-deinit branch from 1b1d9de to a6bc0e6 Compare September 18, 2024 03:53
@ktoso
Copy link
Contributor Author

ktoso commented Sep 18, 2024

@swift-ci please test

@ktoso
Copy link
Contributor Author

ktoso commented Sep 19, 2024

@swift-ci please test

@ktoso ktoso added the concurrency Feature: umbrella label for concurrency language features label Sep 19, 2024
@ktoso
Copy link
Contributor Author

ktoso commented Sep 20, 2024

@swift-ci please test

@ktoso
Copy link
Contributor Author

ktoso commented Sep 22, 2024

@swift-ci please test

1 similar comment
@ktoso
Copy link
Contributor Author

ktoso commented Sep 23, 2024

@swift-ci please test

@ktoso
Copy link
Contributor Author

ktoso commented Sep 23, 2024

@swift-ci please test

@ktoso
Copy link
Contributor Author

ktoso commented Sep 26, 2024

@swift-ci please test

@ktoso
Copy link
Contributor Author

ktoso commented Sep 30, 2024

@swift-ci please test

@ktoso
Copy link
Contributor Author

ktoso commented Sep 30, 2024

@swift-ci please smoke test macOS

1 similar comment
@ktoso
Copy link
Contributor Author

ktoso commented Oct 2, 2024

@swift-ci please smoke test macOS

@ktoso
Copy link
Contributor Author

ktoso commented Oct 2, 2024

@swift-ci please test macOS

@ktoso
Copy link
Contributor Author

ktoso commented Oct 3, 2024

@swift-ci please test macOS

@ktoso ktoso merged commit dea3b59 into swiftlang:main Oct 3, 2024
5 checks passed
@ktoso ktoso deleted the wip-experimental-isolated-deinit branch October 3, 2024 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
concurrency Feature: umbrella label for concurrency language features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants