Skip to content

Fix data race in WithContext with cancelled parent#20

Open
azdagron wants to merge 1 commit intogo-tomb:v2from
azdagron:fix-with-context-race
Open

Fix data race in WithContext with cancelled parent#20
azdagron wants to merge 1 commit intogo-tomb:v2from
azdagron:fix-with-context-race

Conversation

@azdagron
Copy link

@azdagron azdagron commented May 31, 2018

If the parent context is already cancelled, or cancelled before WithContext returns, there is a data race that can happen between the goroutine that is spawned to detect when the parent context is cancelled or the tomb is dying, which calls Kill() and the code in WithContext that updates the children.

This change fixes the race by not kicking off the goroutine that waits for parent cancellation until after the rest of the initialization has been completed.

@azdagron
Copy link
Author

azdagron commented May 16, 2019

Project is 💀

EDIT (after @niemeyer's gentle rebuke): I apologize for the above comment. I have no excuse.

@azdagron azdagron closed this May 16, 2019
@niemeyer
Copy link
Contributor

Sorry for not responding timely. Project is used in several large and critical production systems. Being responsible for those systems is what keeps maintainers busy elsewhere.

@azdagron
Copy link
Author

I apologize if I came off snarky. That wasn't my intent. I hadn't seen any traction on any PR's for quite some time and assumed that this project was no longer actively maintained. My comment wasn't intended to be a critique of the maintainers.

@azdagron
Copy link
Author

I am more than happy to reopen this. There is at least one other race condition that I found that I would be happy to address in a future PR if there is interest.

@niemeyer
Copy link
Contributor

Okay, let's reopen it then, thanks. Will have a look next week once I'm back from the current event. Please ping if I miss it.

Certainly happy to take the other PR as well, if there is a known race. Thanks for that too.

@niemeyer niemeyer reopened this May 16, 2019
@azdagron azdagron force-pushed the fix-with-context-race branch from 32a061e to 5909db5 Compare January 19, 2023 21:50
@azdagron azdagron changed the title Fix WithContext race Fix data race in WithContext with cancelled parent Jan 19, 2023
@azdagron
Copy link
Author

I just remembered I had this open :) Took a fresh look after these few years and verified that the data race still existed. I think my new solution in the latest commit is more correct.

@azdagron
Copy link
Author

I've also updated the PR description and title to match the new solution.

If the parent context is already cancelled, or cancelled before
WithContext returns, there is a data race that can happen between the
goroutine that is spawned to detect when the parent context is cancelled
or the tomb is dying, which calls Kill() and the code in WithContext
that updates the children.

This change fixes the race by not kicking off the goroutine that waits
for parent cancellation until after the rest of the initialization has
been completed.
@azdagron azdagron force-pushed the fix-with-context-race branch from 5909db5 to 3850328 Compare January 20, 2023 15:55
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