up: fix various race/deadlock conditions on exit#10934
Merged
milas merged 2 commits intodocker:mainfrom Aug 31, 2023
Merged
Conversation
milas
requested review from
StefanScherer,
glours,
laurazard,
ndeloof,
nicksieger and
ulyssessouza
and removed request for
a team
milas
force-pushed
the
fix-stop-up
branch
3 times, most recently
from
b931158 to
4347d31
Compare
If running `up` in foreground mode (i.e. not `-d`), when exiting via `Ctrl-C`, Compose stops all the services it launched directly as part of that `up` command. In one of the E2E tests (`TestUpDependenciesNotStopped`), this was occasionally flaking because the stop behavior was racy: the return might not block on the stop operation because it gets added to the error group in a goroutine. As a result, it was possible for no services to get terminated on exit. There were a few other related pieces here that I uncovered and tried to fix while stressing this. For example, the printer could cause a deadlock if an event was sent to it after it stopped. Also, an error group wasn't really appropriate here; each goroutine is a different operation for printing, signal-handling, etc. If one part fails, we don't actually want printing to stop, for example. This has been switched to a `multierror.Group`, which has the same API but coalesces errors instead of canceling a context the moment the first one fails and returning that single error. Signed-off-by: Milas Bowman <[email protected]>
ndeloof
approved these changes
Aug 28, 2023
Signed-off-by: Milas Bowman <[email protected]>
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #10934 +/- ##
==========================================
+ Coverage 58.24% 58.28% +0.04%
==========================================
Files 123 123
Lines 10864 10882 +18
==========================================
+ Hits 6328 6343 +15
Misses 3916 3916
- Partials 620 623 +3
☔ View full report in Codecov by Sentry. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What I did
If running
upin foreground mode (i.e. not-d), when exiting viaCtrl-C, Compose stops all theservices it launched directly as part of that
upcommand.In one of the E2E tests (
TestUpDependenciesNotStopped), this was occasionally flaking because the stopbehavior was racy: the return might not block on
the stop operation because it gets added to the
error group in a goroutine. As a result, it was
possible for no services to get terminated on exit.
There were a few other related pieces here that
I uncovered and tried to fix while stressing this. For example, the printer could cause a deadlock if an event was sent to it after it stopped.
Also, an error group wasn't really appropriate here; each goroutine is a different operation for printing, signal-handling, etc. If one part fails, we don't
actually want printing to stop, for example. This has been switched to a
multierror.Group, which has the same API but coalesces errors instead of canceling a context the moment the first one fails and returning that single error.(not mandatory) A picture of a cute animal, if possible in relation to what you did
