Skip to content

Don't block on STDIN open#2551

Merged
dmcgowan merged 1 commit intocontainerd:masterfrom
crosbymichael:stdin-block
Aug 28, 2018
Merged

Don't block on STDIN open#2551
dmcgowan merged 1 commit intocontainerd:masterfrom
crosbymichael:stdin-block

Conversation

@crosbymichael
Copy link
Copy Markdown
Member

@crosbymichael crosbymichael commented Aug 15, 2018

This was found testing other runtime shims that are faster than runc(no
containerization). This is a race that can cause the shim to block
forever. It's not an issue for out/err because we open both sides of
the pipe, but for stdin, it expects the client to have it opened.

Signed-off-by: Michael Crosby [email protected]

@mlaventure
Copy link
Copy Markdown
Contributor

mlaventure commented Aug 17, 2018

breaks attach?

--- FAIL: TestContainerAttachProcess (1.34s)
	container_linux_test.go:715: write /proc/self/fd/59: broken pipe
	container_linux_test.go:723: write /proc/self/fd/59: broken pipe
	container_linux_test.go:745: expected output "hello\nhello\n" but received ""

@crosbymichael
Copy link
Copy Markdown
Member Author

crosbymichael commented Aug 17, 2018

@mlaventure ya, its related. Still trying to figure this out.... fifos can be a pain

@crosbymichael crosbymichael added this to the 1.2 milestone Aug 24, 2018
@fuweid
Copy link
Copy Markdown
Member

fuweid commented Aug 26, 2018

@crosbymichael I checked the runtime/v1/linux/proc/exec.go and found that the failure is caused by the fifoCtx's cancel. With your change, the defer cancel might be called before the os.Open. The goroutine in fifo will be going into the f.Close(). That is why we see the broken pipe. In my opinion, we can remove the timeout ctx here.

but for stdin, it expects the client to have it opened.

For this question, I see that the shim service always create fifo-stdin with WRONLY mode if the stdin is not empty. would it be blocked forever, if the client doesn't open it? If we remove the timeout ctx, I think this problem will be solved. 😄

@crosbymichael
Don't block on STDIN open
bc1ff51 
This was found testing other runtime shims that are faster than runc(no
containerization).  This is a race that can cause the shim to block
forever.  It's not an issue for out/err because we open both sides of
the pipe, but for stdin, it expects the client to have it opened.

Signed-off-by: Michael Crosby <[email protected]>
@crosbymichael
Copy link
Copy Markdown
Member Author

crosbymichael commented Aug 27, 2018

Thanks @fuweid !

@fuweid
Copy link
Copy Markdown
Member

fuweid commented Aug 27, 2018

😄

@dmcgowan
Copy link
Copy Markdown
Member

dmcgowan commented Aug 27, 2018

LGTM

@dmcgowan dmcgowan mentioned this pull request Aug 28, 2018
Merged
estesp
estesp approved these changes Aug 28, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@dmcgowan dmcgowan merged commit ce1161f into containerd:master Aug 28, 2018
@Random-Liu Random-Liu mentioned this pull request Jan 23, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@estesp estesp estesp approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.2

Development

Successfully merging this pull request may close these issues.

5 participants

@crosbymichael @mlaventure @fuweid @dmcgowan @estesp