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 exec start api with detach and AttachStdin at same time. fixes #2… #20647

Merged
merged 1 commit into from
Feb 25, 2016

Conversation

coolljt0725
Copy link
Contributor

If create a exec command with AttachStdin on and start with Detach=true
this will cause daemon crash.
This only happen when use API
fixes #20638

Signed-off-by: Lei Jitang [email protected]

/cc @KostyaSha

@calavera
Copy link
Contributor

LGTM


b, err = readBody(body)
comment := check.Commentf("response body: %s", b)
c.Assert(err, checker.IsNil, comment)
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a new daemon request here to make sure it's not crashed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cpuguy83 In this test, there is no need to add a new daemon request here, because if the daemon is crashed, _, body, err := sockRequestRaw("POST", fmt.Sprintf("/exec/%s/start", createResp.ID), strings.NewReader({"Detach": true}), "application/json") will return error.

Copy link
Member

Choose a reason for hiding this comment

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

I understand that will probably happen, but just want to verify by sending a subsequent request to make sure it's still alive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cpuguy83 after some more thoughts, it's better to have this check, I'll do

@coolljt0725
Copy link
Contributor Author

@cpuguy83 @calavera updated

@cpuguy83
Copy link
Member

LGTM

calavera added a commit that referenced this pull request Feb 25, 2016
Fix exec start api with detach and AttachStdin at same time. fixes #2
@calavera calavera merged commit cee4ff1 into moby:master Feb 25, 2016
@coolljt0725 coolljt0725 deleted the fix_20638 branch February 25, 2016 08:01
@tiborvass tiborvass mentioned this pull request Mar 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exec start kills daemon with nil reference
5 participants