-
Notifications
You must be signed in to change notification settings - Fork 582
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
test: Fix race condition in provisionerd on cleanup #322
Conversation
These goroutines could be ran after the pipe has already been closed. I'm not certain this resolves this specific leak: https://github.com/coder/coder/runs/5249481202?check_suite_focus=true#step:7:186 ...but I find it likely.
Codecov Report
@@ Coverage Diff @@
## main #322 +/- ##
==========================================
+ Coverage 63.61% 67.46% +3.84%
==========================================
Files 69 140 +71
Lines 786 7407 +6621
Branches 77 77
==========================================
+ Hits 500 4997 +4497
- Misses 271 1900 +1629
- Partials 15 510 +495
Continue to review full report at Codecov.
|
ctx, cancelFunc := context.WithCancel(context.Background()) | ||
t.Cleanup(cancelFunc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting... it's not clear to me how this will fix it, but I hope it does 🤞
It seems safe in any case, and it seems reasonable for this to be outside the func that starts the server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely sure either, but t.Cleanup
certainly shouldn't be called async from my POV anyways.
Thanks for investigating this race, @kylecarbs ! |
These goroutines could be ran after the pipe has already been closed.
I'm not certain this resolves this specific leak:
https://github.com/coder/coder/runs/5249481202?check_suite_focus=true#step:7:186
...but I find it likely.