Skip to content
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

fix race condition, waiting for containers when one exit #10725

Merged
merged 1 commit into from Jun 20, 2023

Conversation

ndeloof
Copy link
Contributor

@ndeloof ndeloof commented Jun 20, 2023

What I did
as a container exits with error, we must cancel but the goroutine waiting for expected container to be started used to get stuck waiting. Need to stop waiting when context is cancelled

Related issue
fixes #10707
fixes #10718

(not mandatory) A picture of a cute animal, if possible in relation to what you did

@codecov
Copy link

codecov bot commented Jun 20, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.04 🎉

Comparison is base (83b2433) 58.94% compared to head (0de121f) 58.99%.

Additional details and impacted files
@@            Coverage Diff             @@
##               v2   #10725      +/-   ##
==========================================
+ Coverage   58.94%   58.99%   +0.04%     
==========================================
  Files         112      112              
  Lines        9749     9752       +3     
==========================================
+ Hits         5747     5753       +6     
+ Misses       3413     3410       -3     
  Partials      589      589              
Impacted Files Coverage Δ
pkg/compose/run.go 53.33% <ø> (ø)
pkg/compose/dependencies.go 87.24% <100.00%> (+0.48%) ⬆️

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ndeloof ndeloof requested review from a team, nicksieger, StefanScherer, ulyssessouza, glours, milas and laurazard and removed request for a team June 20, 2023 08:35
Copy link
Contributor

@glours glours left a comment

Choose a reason for hiding this comment

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

LGTM

@fho
Copy link

fho commented Jun 20, 2023

it also should fix: #10648,
maybe also #10661

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
@ndeloof
Copy link
Contributor Author

ndeloof commented Jun 20, 2023

@fho would you have a chance to test this against #10648 and #10661 to confirm those are all the same issue ?

@fho
Copy link

fho commented Jun 20, 2023

@fho would you have a chance to test this against #10648 and #10661 to confirm those are all the same issue ?

@ndeloof
I already tested #10648 with this change, it fixes the issue.
With the described steps in #1661 I can not reproduce an issue.

@jawys
Copy link

jawys commented Jun 28, 2023

Hello there!

It took me 3 days of troubleshooting/purging/reinstalling docker-desktop to finally find this issue because compose also stopped throwing error messages which could have shortened my odyssey:

compose-not-responding+stuck-forever.mp4

@ndeloof Could we please somehow trigger a release of Docker Desktop whenever a new Compose version is released so that always recent versions get shipped??
At our company compose is crucial for development, so the recent experience was real pain until I somehow found this related isse.

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