Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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 leijitang@huawei.com

/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
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exec start kills daemon with nil reference
5 participants