Skip to content

fix data race in build-complete handler #3352

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
Merged
merged 1 commit into from
Mar 12, 2021
Merged

fix data race in build-complete handler #3352

merged 1 commit into from
Mar 12, 2021

Conversation

tomerd
Copy link
Contributor

@tomerd tomerd commented Mar 12, 2021

motivation: fix data race

changes: guard access to progressAnimation with the queue

@tomerd
fix data race in build-complete handler
d406cca 
motivation: fix data race

changes: guard access to progressAnimation with the queue
@tomerd
Copy link
Contributor Author

tomerd commented Mar 12, 2021

@swift-ci please smoke test

@tomerd tomerd added the ready Author believes the PR is ready to be merged & any feedback has been addressed label Mar 12, 2021
@tomerd tomerd self-assigned this Mar 12, 2021
abertelrud
abertelrud approved these changes Mar 12, 2021
View reviewed changes
Copy link
Contributor

@abertelrud abertelrud left a comment

Choose a reason for hiding this comment

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

Seems like a good short-term fix. In the long term I would expect ProgressAnimation to be threadsafe (not familiar at all with that class, so that might not be a valid assumption, but that seems like a reasonable expectation from the outside...)

@tomerd
Copy link
Contributor Author

tomerd commented Mar 12, 2021

Seems like a good short-term fix. In the long term I would expect ProgressAnimation to be threadsafe (not familiar at all with that class, so that might not be a valid assumption, but that seems like a reasonable expectation from the outside...)

totally agree, this entire part of the code needs to be cleaned up in that sense, so this fix is more to bring the new build-complete handler in line with the rest of this code is doing

@tomerd tomerd merged commit 230ea26 into swiftlang:main Mar 12, 2021
@tomerd tomerd deleted the fix/data-race branch March 12, 2021 18:58
yim-lee pushed a commit to yim-lee/swift-package-manager that referenced this pull request Mar 12, 2021
@tomerd
fix data race in build-complete handler (swiftlang#3352)
dd49f80 
motivation: fix data race

changes: guard access to progressAnimation with the queue
@tomerd tomerd mentioned this pull request May 7, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abertelrud abertelrud abertelrud approved these changes

@friedbunny friedbunny Awaiting requested review from friedbunny

@neonichu neonichu Awaiting requested review from neonichu

Assignees

@tomerd tomerd

Labels
ready Author believes the PR is ready to be merged & any feedback has been addressed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tomerd @abertelrud