Skip to content

Set isRunning of Process to false before calling the termination handler #3000

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

Merged

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented May 25, 2021

Otherwise, we end up in a race condition where isRunning might still be true when accessed from inside the terminationHandler. If this is the case, accessing terminationReason or terminationStatus inside the termination handler crashes because their precondition that the task has finished are not satisfied.

We are experiencing this flaky behavior in SourceKit-LSP’s crash recovery tests on Linux.

Fixes rdar://78035044

…n handler

Otherwise, we end up in a race condition where `isRunning` might still be `true` when accessed from inside the `terminationHandler`. If this is the case, accessing `terminationReason` or `terminationStatus` inside the termination handler crashes because their precondition that the task has finished are not satisfied.

Fixes rdar://78035044
@ahoppen ahoppen requested a review from spevans May 25, 2021 14:35
@ahoppen
Copy link
Member Author

ahoppen commented May 25, 2021

@swift-ci Please test

Copy link
Contributor

@spevans spevans left a comment

Choose a reason for hiding this comment

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

LGTM

@spevans spevans merged commit 8893f51 into swiftlang:main May 26, 2021
@spevans
Copy link
Contributor

spevans commented May 26, 2021

@ahoppen Might be good to backport to 5.4 as well since this fixes an obvious bug

@ahoppen
Copy link
Member Author

ahoppen commented May 26, 2021

@spevans Do you mean to backport to 5.5 or to 5.4 and 5.5? Only back porting to 5.4 doesn’t make sense to me, unless I’m missing something about the corelibs-foundation branching scheme.

@ahoppen ahoppen deleted the pr/is-not-running-before-termination-handler branch May 26, 2021 10:02
@spevans
Copy link
Contributor

spevans commented May 26, 2021

Sorry, yes I meant the 5.5 branch.

@ahoppen
Copy link
Member Author

ahoppen commented May 26, 2021

OK, set up a 5.5 PR here: #3002

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants