-
Notifications
You must be signed in to change notification settings - Fork 5.7k
refactor(attach): improve attach error handling and cleanup #13488
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
Changes from all commits
8da3c30
5825bcb
aad4071
1036fbb
e83b48b
317e170
32cd647
8af7749
0d78f61
2ddc252
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,10 +24,9 @@ import ( | |
| "strings" | ||
|
|
||
| "github.com/compose-spec/compose-go/v2/types" | ||
| "github.com/docker/cli/cli/streams" | ||
| containerType "github.com/docker/docker/api/types/container" | ||
| "github.com/docker/docker/pkg/stdcopy" | ||
| "github.com/moby/term" | ||
| "github.com/sirupsen/logrus" | ||
|
|
||
| "github.com/docker/compose/v5/pkg/api" | ||
| "github.com/docker/compose/v5/pkg/utils" | ||
|
|
@@ -49,15 +48,18 @@ func (s *composeService) attach(ctx context.Context, project *types.Project, lis | |
| names = append(names, getContainerNameWithoutProject(c)) | ||
| } | ||
|
|
||
| _, _ = fmt.Fprintf(s.stdout(), "Attaching to %s\n", strings.Join(names, ", ")) | ||
| _, err = fmt.Fprintf(s.stdout(), "Attaching to %s\n", strings.Join(names, ", ")) | ||
| if err != nil { | ||
| logrus.Debugf("failed to write attach message: %v", err) | ||
| } | ||
|
|
||
| for _, ctr := range containers { | ||
| err := s.attachContainer(ctx, ctr, listener) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
| return containers, err | ||
| return containers, nil | ||
| } | ||
|
|
||
| func (s *composeService) attachContainer(ctx context.Context, container containerType.Summary, listener api.ContainerEventListener) error { | ||
|
|
@@ -91,78 +93,59 @@ func (s *composeService) doAttachContainer(ctx context.Context, service, id, nam | |
| }) | ||
| }) | ||
|
|
||
| _, _, err = s.attachContainerStreams(ctx, id, inspect.Config.Tty, nil, wOut, wErr) | ||
| return err | ||
| } | ||
|
|
||
| func (s *composeService) attachContainerStreams(ctx context.Context, container string, tty bool, stdin io.ReadCloser, stdout, stderr io.WriteCloser) (func(), chan bool, error) { | ||
| detached := make(chan bool) | ||
| restore := func() { /* noop */ } | ||
| if stdin != nil { | ||
| in := streams.NewIn(stdin) | ||
| if in.IsTerminal() { | ||
| state, err := term.SetRawTerminal(in.FD()) | ||
| if err != nil { | ||
| return restore, detached, err | ||
| } | ||
| restore = func() { | ||
| term.RestoreTerminal(in.FD(), state) //nolint:errcheck | ||
| } | ||
| } | ||
| } | ||
|
|
||
| streamIn, streamOut, err := s.getContainerStreams(ctx, container) | ||
| err = s.attachContainerStreams(ctx, id, inspect.Config.Tty, wOut, wErr) | ||
| if err != nil { | ||
| return restore, detached, err | ||
| return err | ||
| } | ||
|
|
||
| go func() { | ||
| <-ctx.Done() | ||
| if stdin != nil { | ||
| stdin.Close() //nolint:errcheck | ||
| } | ||
| }() | ||
| return nil | ||
| } | ||
|
|
||
| if streamIn != nil && stdin != nil { | ||
| go func() { | ||
| _, err := io.Copy(streamIn, stdin) | ||
| var escapeErr term.EscapeError | ||
| if errors.As(err, &escapeErr) { | ||
| close(detached) | ||
| } | ||
| }() | ||
| func (s *composeService) attachContainerStreams(ctx context.Context, container string, tty bool, stdout, stderr io.WriteCloser) error { | ||
| streamOut, err := s.getContainerStreams(ctx, container) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if stdout != nil { | ||
| go func() { | ||
| defer stdout.Close() //nolint:errcheck | ||
| defer stderr.Close() //nolint:errcheck | ||
| defer streamOut.Close() //nolint:errcheck | ||
| defer func() { | ||
| if err := stdout.Close(); err != nil { | ||
| logrus.Debugf("failed to close stdout: %v", err) | ||
| } | ||
| if err := stderr.Close(); err != nil { | ||
| logrus.Debugf("failed to close stderr: %v", err) | ||
| } | ||
| if err := streamOut.Close(); err != nil { | ||
| logrus.Debugf("failed to close stream output: %v", err) | ||
| } | ||
| }() | ||
|
|
||
| var err error | ||
| if tty { | ||
| io.Copy(stdout, streamOut) //nolint:errcheck | ||
| _, err = io.Copy(stdout, streamOut) | ||
| } else { | ||
| stdcopy.StdCopy(stdout, stderr, streamOut) //nolint:errcheck | ||
| _, err = stdcopy.StdCopy(stdout, stderr, streamOut) | ||
| } | ||
| if err != nil && !errors.Is(err, io.EOF) { | ||
| logrus.Debugf("stream copy error for container %s: %v", container, err) | ||
| } | ||
| }() | ||
| } | ||
| return restore, detached, nil | ||
| return nil | ||
| } | ||
|
|
||
| func (s *composeService) getContainerStreams(ctx context.Context, container string) (io.WriteCloser, io.ReadCloser, error) { | ||
| var stdout io.ReadCloser | ||
| var stdin io.WriteCloser | ||
| func (s *composeService) getContainerStreams(ctx context.Context, container string) (io.ReadCloser, error) { | ||
| cnx, err := s.apiClient().ContainerAttach(ctx, container, containerType.AttachOptions{ | ||
| Stream: true, | ||
| Stdin: true, | ||
| Stdout: true, | ||
| Stderr: true, | ||
| Logs: false, | ||
| DetachKeys: s.configFile().DetachKeys, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious; wouldn't we need the detach-keys? i.e., does this mean it's no longer possible to detach from a container, or is this not used for interactive use?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. only used to get output logs. For interactive mode, we rely on docker/cli RunExec and RunAttach |
||
| Stream: true, | ||
| Stdin: false, | ||
| Stdout: true, | ||
| Stderr: true, | ||
| Logs: false, | ||
| }) | ||
| if err == nil { | ||
| stdout = ContainerStdout{HijackedResponse: cnx} | ||
| stdin = ContainerStdin{HijackedResponse: cnx} | ||
| return stdin, stdout, nil | ||
| stdout := ContainerStdout{HijackedResponse: cnx} | ||
| return stdout, nil | ||
| } | ||
|
|
||
| // Fallback to logs API | ||
|
|
@@ -172,7 +155,7 @@ func (s *composeService) getContainerStreams(ctx context.Context, container stri | |
| Follow: true, | ||
| }) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| return nil, err | ||
| } | ||
| return stdin, logs, nil | ||
| return logs, nil | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly wondering how useful these are, other than "please the linter"; logging here would try to write to
stderr, which may also fail