Fix hang when container fails to start by laurazard · Pull Request #5060 · docker/cli

laurazard · 2024-05-05T22:21:34Z

- What I did

When running a container, if it fails to start, we immediately cancel the context used for waitExitOrRemoved and, if running the container with --rm, wait on the channel:

cli/cli/command/container/run.go

Lines 196 to 205 in 870ad7f

    
           if attach { 
        
           	cancelFun() 
        
           	<-errCh 
        
           } 
        
           reportError(stderr, "run", err.Error(), false) 
        
           if copts.autoRemove { 
        
           	// wait container to be removed 
        
           	<-statusChan 
        
           }

However, in 840016e, we added the following line

cli/cli/command/container/utils.go

Lines 40 to 41 in 647ccf3

    
           case <-ctx.Done(): 
        
           	return

but in this case, we return from the goroutine without notifying the caller, which causes the caller to hang.

- How I did it

We should not be cancelling the context used to wait on the container exit/removal in this case, and it's only happening by accident due to this line, which is there for other reasons

cli/cli/command/container/run.go

Lines 196 to 199 in 05cba3f

    
           if attach { 
        
           	cancelFun() 
        
           	<-errCh 
        
           }

So I used a new context for the waitExitOrRemoved call.

Also, close the returned channel to unblock any caller when we hit this branch, and added another close to a case down where we added another return without notifying the caller.

- How to verify it

Reproduce such as:

#!/bin/bash
while :; do
	docker run --rm --entrypoint invalidcommand alpine param
done

- Description for the changelog

Fix issue where the CLI process would sometimes hang when a container failed to start.

- A picture of a cute animal (not mandatory but encouraged)

laurazard · 2024-05-05T22:22:31Z

Should also add a test that would catch a regression such as this, but I'll leave that for tomorrow 😅

codecov-commenter · 2024-05-05T22:23:35Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.07%. Comparing base (647ccf3) to head (8d6e571).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #5060   +/-   ##
=======================================
  Coverage   61.06%   61.07%           
=======================================
  Files         295      295           
  Lines       20667    20670    +3     
=======================================
+ Hits        12621    12624    +3     
  Misses       7147     7147           
  Partials      899      899

thaJeztah · 2024-05-06T06:24:45Z

Will this affect client-side handling of --rm (API 1.25 and older)? I noticed that's handled in this area of the code, and I recall there was some rather complex logic around this.

laurazard · 2024-05-06T09:38:40Z

I don't think it should, that check/path happens before the changes in this patch/the other changes right?

cli/cli/command/container/utils.go

Lines 26 to 28 in 647ccf3

    
           if versions.LessThan(apiClient.ClientVersion(), "1.30") { 
        
           	return legacyWaitExitOrRemoved(ctx, apiClient, containerID, waitRemove) 
        
           }

cli/command/container/utils.go

e2e/container/run_test.go

cli/command/container/run.go

vvoland

LGTM; left a small nit, but feel free to ignore 😅

cli/command/container/utils.go

Signed-off-by: Laura Brehm <[email protected]>

thaJeztah

LGTM

We should probably look at some of our tests to make sure cleanup happens; it looks like icmd.RunCmd may be broken (as it doesn't set the timeout on the cmd that's passed to icmd.WaitOnCmd). But while doing a little test (running sleep 10 instead of invalid command) I noticed that the container is not cleaned up when the test completes; we may either need some cleanup step as part of tests, or as part of the setup of the e2e tests 🤔

laurazard · 2024-05-07T10:40:31Z

Agree! I'll write a ticket somewhere for us to remember to do that, should look at all the tests and check that.

thaJeztah · 2024-05-07T10:51:15Z

Thanks! Yes, looks like a tracking ticket won't hurt. In most cases it probably shouldn't be problematic, but it could explain some flakiness (containers from other tests still being around, causing tests to be inconsistent).

In either case, not critical for this PR; I think this PR should be good to go

laurazard added the kind/bugfix PR's that fix bugs label May 5, 2024

laurazard requested a review from vvoland May 5, 2024 22:21

laurazard self-assigned this May 5, 2024

laurazard force-pushed the fix-hang-ctx branch from cd8f875 to 05cba3f Compare May 6, 2024 11:21

krissetto reviewed May 6, 2024

View reviewed changes

cli/command/container/utils.go Outdated Show resolved Hide resolved

thaJeztah reviewed May 6, 2024

View reviewed changes

e2e/container/run_test.go Show resolved Hide resolved

e2e/container/run_test.go Outdated Show resolved Hide resolved

laurazard force-pushed the fix-hang-ctx branch 2 times, most recently from f431514 to 109a06e Compare May 6, 2024 13:02

laurazard requested review from krissetto and thaJeztah May 6, 2024 13:51

vvoland reviewed May 7, 2024

View reviewed changes

cli/command/container/run.go Outdated Show resolved Hide resolved

laurazard force-pushed the fix-hang-ctx branch 3 times, most recently from 109a06e to 32d7da6 Compare May 7, 2024 09:34

laurazard requested a review from vvoland May 7, 2024 09:35

vvoland approved these changes May 7, 2024

View reviewed changes

cli/command/container/utils.go Show resolved Hide resolved

laurazard added 2 commits May 7, 2024 11:22

Fix hang when container fails to start

31644d5

Signed-off-by: Laura Brehm <[email protected]>

Add e2e tests for run w/ bad entrypoint

8d6e571

Signed-off-by: Laura Brehm <[email protected]>

laurazard force-pushed the fix-hang-ctx branch from 32d7da6 to 8d6e571 Compare May 7, 2024 10:22

thaJeztah approved these changes May 7, 2024

View reviewed changes

thaJeztah added this to the 27.0.0 milestone May 7, 2024

thaJeztah added the process/cherry-pick/26.1 label May 7, 2024

thaJeztah merged commit 0bdc20e into docker:master May 7, 2024

laurazard mentioned this pull request May 7, 2024

[26.1 backport] Fix hang when container fails to start #5062

Merged

thaJeztah removed the process/cherry-pick/26.1 label Oct 8, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix hang when container fails to start#5060

Fix hang when container fails to start#5060
thaJeztah merged 2 commits intodocker:masterfrom
laurazard:fix-hang-ctx

laurazard commented May 5, 2024 •

edited

Loading

Uh oh!

laurazard commented May 5, 2024

Uh oh!

codecov-commenter commented May 5, 2024 •

edited

Loading

Uh oh!

thaJeztah commented May 6, 2024

Uh oh!

laurazard commented May 6, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vvoland left a comment

Uh oh!

Uh oh!

thaJeztah left a comment

Uh oh!

laurazard commented May 7, 2024

Uh oh!

thaJeztah commented May 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	if attach {
	cancelFun()
	<-errCh
	}

	reportError(stderr, "run", err.Error(), false)
	if copts.autoRemove {
	// wait container to be removed
	<-statusChan
	}

Conversation

laurazard commented May 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laurazard commented May 5, 2024

Uh oh!

codecov-commenter commented May 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thaJeztah commented May 6, 2024

Uh oh!

laurazard commented May 6, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vvoland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

laurazard commented May 7, 2024

Uh oh!

thaJeztah commented May 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

laurazard commented May 5, 2024 •

edited

Loading

codecov-commenter commented May 5, 2024 •

edited

Loading