8000 run: correctly handle only STDIN attached by laurazard · Pull Request #5662 · docker/cli · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

run: correctly handle only STDIN attached #5662

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Dec 9, 2024
Merged

run: correctly handle only STDIN attached #5662

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
aee9eeb
run: return error code when only STDIN attached
laurazard Nov 29, 2024
8431298
run: cleanup – use `attached` where applicable
laurazard Dec 2, 2024
446f36c
run: cleanup – move "detached" early exit earlier
laurazard Dec 2, 2024
a3d9fc4
run: cleanup – remove `errCh` nil check
laurazard Dec 2, 2024
30c4637
run: don't hang if only attaching STDIN
laurazard Dec 2, 2024
7dab597
tests: cleanup comment
laurazard Dec 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 25 additions & 16 deletions cli/command/container/run.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,15 +161,15 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption
waitDisplayID chan struct{}
errCh chan error
)
if !config.AttachStdout && !config.AttachStderr {
attach := config.AttachStdin || config.AttachStdout || config.AttachStderr
if !attach {
// Make this asynchronous to allow the client to write to stdin before having to read the ID
waitDisplayID = make(chan struct{})
go func() {
defer close(waitDisplayID)
_, _ = fmt.Fprintln(stdout, containerID)
}()
}
attach := config.AttachStdin || config.AttachStdout || config.AttachStderr
if attach {
detachKeys := dockerCli.ConfigFile().DetachKeys
if runOpts.detachKeys != "" {
Expand Down Expand Up @@ -215,14 +215,22 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption
return toStatusError(err)
}

if (config.AttachStdin || config.AttachStdout || config.AttachStderr) && config.Tty && dockerCli.Out().IsTerminal() {
// Detached mode: wait for the id to be displayed and return.
if !attach {
// Detached mode
<-waitDisplayID
return nil
}

if config.Tty && dockerCli.Out().IsTerminal() {
Copy link
Member

Choose a reason for hiding this comment

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

Curious (but perhaps for separate) if we should check if either STDOUT or STDERR is a terminal (for cases where one of them may be redirected). That's mostly with an (unrelated) discussion in the back of my head, where we didn't take that case into account.

if err := MonitorTtySize(ctx, dockerCli, containerID, false); err != nil {
_, _ = fmt.Fprintln(stderr, "Error monitoring TTY size:", err)
}
}

if errCh != nil {
if err := <-errCh; err != nil {
select {
case err := <-errCh:
if err != nil {
if _, ok := err.(term.EscapeError); ok {
// The user entered the detach escape sequence.
return nil
Expand All @@ -231,19 +239,20 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption
logrus.Debugf("Error hijack: %s", err)
return err
}
status := <-statusChan
if status != 0 {
return cli.StatusError{StatusCode: status}
}
case status := <-statusChan:
// notify hijackedIOStreamer that we're exiting and wait
// so that the terminal can be restored.
cancelFun()
<-errCh
if status != 0 {
return cli.StatusError{StatusCode: status}
}
}

// Detached mode: wait for the id to be displayed and return.
if !config.AttachStdout && !config.AttachStderr {
// Detached mode
<-waitDisplayID
return nil
}

status := <-statusChan
if status != 0 {
return cli.StatusError{StatusCode: status}
}
return nil
}

Expand Down
1 change: 0 additions & 1 deletion e2e/container/attach_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ func TestAttachInterrupt(t *testing.T) {
// todo(laurazard): make this test work w/ dind over ssh
skip.If(t, strings.Contains(os.Getenv("DOCKER_HOST"), "ssh://"))

// if
result := icmd.RunCommand("docker", "run", "-d", fixtures.AlpineImage,
"sh", "-c", "trap \"exit 33\" SIGINT; for i in $(seq 100); do sleep 0.1; done; exit 34")
result.Assert(t, icmd.Success)
Expand Down
35 changes: 35 additions & 0 deletions e2e/container/run_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@ package container
import (
"bytes"
"fmt"
"os/exec"
"strings"
"syscall"
"testing"
"time"

"github.com/creack/pty"
"github.com/docker/cli/e2e/internal/fixtures"
"github.com/docker/cli/internal/test/environment"
"github.com/docker/docker/api/types/versions"
Expand Down Expand Up @@ -38,6 +40,39 @@ func TestRunAttachedFromRemoteImageAndRemove(t *testing.T) {
golden.Assert(t, result.Stderr(), "run-attached-from-remote-and-remove.golden")
}

func TestRunAttach(t *testing.T) {
skip.If(t, environment.RemoteDaemon())
t.Parallel()
Copy link
Member

Choose a reason for hiding this comment

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

Looks like these changes landed in the wrong commit (probably you wanted to put them in the commit before this 🙈

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ahhhhhh 🤦‍♀️ that's what I get for trying to do 3 things at the same time.


streams := []string{"stdin", "stdout", "stderr"}
for _, stream := range streams {
t.Run(stream, func(t *testing.T) {
t.Parallel()
c := exec.Command("docker", "run", "-a", stream, "--rm", "alpine",
"sh", "-c", "sleep 1 && exit 7")
d := bytes.Buffer{}
c.Stdout = &d
c.Stderr = &d
_, err := pty.Start(c)
assert.NilError(t, err)

done := make(chan error)
go func() {
done <- c.Wait()
}()

select {
case <-time.After(20 * time.Second):
t.Fatal("docker run took too long, likely hang", d.String())
case <-done:
}

assert.Equal(t, c.ProcessState.ExitCode(), 7)
assert.Check(t, is.Contains(d.String(), "exit status 7"))
})
}
}

// Regression test for https://github.com/docker/cli/issues/5053
func TestRunInvalidEntrypointWithAutoremove(t *testing.T) {
environment.SkipIfDaemonNotLinux(t)
Expand Down
Loading
0