Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit40377be

Browse files
committed
fix(agent): Close stdin and stdout separately to fix pty output loss
Fixes#6656Closes#6840
1 parent311327c commit40377be

File tree

5 files changed

+55
-27
lines changed

5 files changed

+55
-27
lines changed

‎agent/agent.go

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1131,19 +1131,32 @@ func (a *agent) handleSSHSession(session ssh.Session) (retErr error) {
11311131
// output being lost. To avoid this, we wait for the output copy to
11321132
// start before waiting for the command to exit. This ensures that the
11331133
// output copy goroutine will be scheduled before calling close on the
1134-
// pty.There is still a risk ofdata loss if a command produces a lot
1135-
//of output, see TestAgent_Session_TTY_HugeOutputIsNotLost (skipped).
1134+
// pty.This shouldn't be needed because of`pty.Dup()` below, but it
1135+
//may not be supported on all platforms.
11361136
outputCopyStarted:=make(chanstruct{})
1137-
ptyOutput:=func() io.Reader {
1137+
ptyOutput:=func() io.ReadCloser {
11381138
deferclose(outputCopyStarted)
1139-
returnptty.Output()
1139+
// Try to dup so we can separate stdin and stdout closure.
1140+
// Once the original pty is closed, the dup will return
1141+
// input/output error once the buffered data has been read.
1142+
stdout,err:=ptty.Dup()
1143+
iferr==nil {
1144+
returnstdout
1145+
}
1146+
// If we can't dup, we shouldn't close
1147+
// the fd since it's tied to stdin.
1148+
returnreadNopCloser{ptty.Output()}
11401149
}
11411150
wg.Add(1)
11421151
gofunc() {
11431152
// Ensure data is flushed to session on command exit, if we
11441153
// close the session too soon, we might lose data.
11451154
deferwg.Done()
1146-
_,_=io.Copy(session,ptyOutput())
1155+
1156+
stdout:=ptyOutput()
1157+
deferstdout.Close()
1158+
1159+
_,_=io.Copy(session,stdout)
11471160
}()
11481161
<-outputCopyStarted
11491162

@@ -1372,6 +1385,11 @@ func (a *agent) handleReconnectingPTY(ctx context.Context, logger slog.Logger, m
13721385
}
13731386
}
13741387

1388+
typereadNopCloserstruct{ io.Reader }
1389+
1390+
// Close implements io.Closer.
1391+
func (rreadNopCloser)Close()error {returnnil }
1392+
13751393
// startReportingConnectionStats runs the connection stats reporting goroutine.
13761394
func (a*agent)startReportingConnectionStats(ctx context.Context) {
13771395
reportStats:=func(networkStatsmap[netlogtype.Connection]netlogtype.Counts) {

‎agent/agent_test.go

Lines changed: 8 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -350,15 +350,8 @@ func TestAgent_Session_TTY_Hushlogin(t *testing.T) {
350350

351351
funcTestAgent_Session_TTY_FastCommandHasOutput(t*testing.T) {
352352
t.Parallel()
353-
ifruntime.GOOS=="windows" {
354-
// This might be our implementation, or ConPTY itself.
355-
// It's difficult to find extensive tests for it, so
356-
// it seems like it could be either.
357-
t.Skip("ConPTY appears to be inconsistent on Windows.")
358-
}
359-
360353
// This test is here to prevent regressions where quickly executing
361-
// commands (with TTY) don'tflush their output to the SSH session.
354+
// commands (with TTY) don'tsync their output to the SSH session.
362355
//
363356
// See: https://github.com/coder/coder/issues/6656
364357
ctx,cancel:=context.WithTimeout(context.Background(),testutil.WaitLong)
@@ -404,20 +397,13 @@ func TestAgent_Session_TTY_FastCommandHasOutput(t *testing.T) {
404397

405398
funcTestAgent_Session_TTY_HugeOutputIsNotLost(t*testing.T) {
406399
t.Parallel()
407-
ifruntime.GOOS=="windows" {
408-
// This might be our implementation, or ConPTY itself.
409-
// It's difficult to find extensive tests for it, so
410-
// it seems like it could be either.
411-
t.Skip("ConPTY appears to be inconsistent on Windows.")
412-
}
413-
t.Skip("This test proves we have a bug where parts of large output on a PTY can be lost after the command exits, skipped to avoid test failures.")
414-
415-
// This test is here to prevent prove we have a bug where quickly executing
416-
// commands (with TTY) don't flush their output to the SSH session. This is
417-
// due to the pty being closed before all the output has been copied, but
418-
// protecting against this requires a non-trivial rewrite of the output
419-
// processing (or figuring out a way to put the pty in a mode where this
420-
// does not happen).
400+
401+
// This test is here to prevent regressions where a command (with or
402+
// without) a large amount of output would not be fully copied to the
403+
// SSH session. On unix systems, this was fixed by duplicating the file
404+
// descriptor of the PTY master and using it for copying the output.
405+
//
406+
// See: https://github.com/coder/coder/issues/6656
421407
ctx,cancel:=context.WithTimeout(context.Background(),testutil.WaitLong)
422408
defercancel()
423409
//nolint:dogsled

‎pty/pty.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,11 @@ type PTY interface {
3131
// The same stream would be used to provide user input: pty.Input().Write(...)
3232
Input()ReadWriter
3333

34+
// Dup returns a new file descriptor for the PTY.
35+
//
36+
// This is useful for closing stdin and stdout separately.
37+
Dup() (*os.File,error)
38+
3439
// Resize sets the size of the PTY.
3540
Resize(heightuint16,widthuint16)error
3641
}

‎pty/pty_other.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"os/exec"
88
"runtime"
99
"sync"
10+
"syscall"
1011

1112
"github.com/creack/pty"
1213
"github.com/u-root/u-root/pkg/termios"
@@ -113,6 +114,20 @@ func (p *otherPty) Resize(height uint16, width uint16) error {
113114
})
114115
}
115116

117+
func (p*otherPty)Dup() (*os.File,error) {
118+
varnewfdint
119+
err:=p.control(p.pty,func(fduintptr)error {
120+
varerrerror
121+
newfd,err=syscall.Dup(int(fd))
122+
returnerr
123+
})
124+
iferr!=nil {
125+
returnnil,err
126+
}
127+
128+
returnos.NewFile(uintptr(newfd),p.pty.Name()),nil
129+
}
130+
116131
func (p*otherPty)Close()error {
117132
p.mutex.Lock()
118133
deferp.mutex.Unlock()

‎pty/pty_windows.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,10 @@ func (p *ptyWindows) Resize(height uint16, width uint16) error {
123123
returnnil
124124
}
125125

126+
func (p*ptyWindows)Dup() (*os.File,error) {
127+
returnnil,xerrors.Errorf("not implemented")
128+
}
129+
126130
func (p*ptyWindows)Close()error {
127131
p.closeMutex.Lock()
128132
deferp.closeMutex.Unlock()

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp