Skip to content

Commit c09083e

Browse files
spikecurtisspikecurtis
committed
Code review fixes
Signed-off-by: Spike Curtis <spike@coder.com>
1 parent 50e3fec commit c09083e

File tree

8 files changed

+44
-30
lines changed

8 files changed

+44
-30
lines changed

‎agent/agent_test.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,6 @@ import (
2424
"testing"
2525
"time"
2626

27-
"github.com/coder/coder/pty"
28-
2927
scp "github.com/bramvdbogaerde/go-scp"
3028
"github.com/google/uuid"
3129
"github.com/pion/udp"
@@ -47,6 +45,7 @@ import (
4745
"github.com/coder/coder/coderd/httpapi"
4846
"github.com/coder/coder/codersdk"
4947
"github.com/coder/coder/codersdk/agentsdk"
48+
"github.com/coder/coder/pty"
5049
"github.com/coder/coder/pty/ptytest"
5150
"github.com/coder/coder/tailnet"
5251
"github.com/coder/coder/tailnet/tailnettest"

‎go.mod

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,7 @@ require (
108108
github.com/hashicorp/terraform-config-inspect v0.0.0-20211115214459-90acf1ca460f
109109
github.com/hashicorp/terraform-json v0.14.0
110110
github.com/hashicorp/yamux v0.0.0-20220718163420-dd80a7ee44ce
111+
github.com/hinshun/vt10x v0.0.0-20220301184237-5011da428d02
111112
github.com/imulab/go-scim/pkg/v2 v2.2.0
112113
github.com/jedib0t/go-pretty/v6 v6.4.0
113114
github.com/jmoiron/sqlx v1.3.5
@@ -174,8 +175,6 @@ require (
174175
tailscale.com v1.32.2
175176
)
176177

177-
require github.com/hinshun/vt10x v0.0.0-20220301184237-5011da428d02 // indirect
178-
179178
require (
180179
cloud.google.com/go/compute v1.18.0 // indirect
181180
cloud.google.com/go/logging v1.6.1 // indirect

‎go.sum

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1086,7 +1086,6 @@ github.com/hashicorp/yamux v0.0.0-20220718163420-dd80a7ee44ce h1:7FO+LmZwiG/eDsB
10861086
github.com/hashicorp/yamux v0.0.0-20220718163420-dd80a7ee44ce/go.mod h1:CtWFDAQgb7dxtzFs4tWbplKIe2jSi3+5vKbgIO0SLnQ=
10871087
github.com/hdevalence/ed25519consensus v0.0.0-20220222234857-c00d1f31bab3 h1:aSVUgRRRtOrZOC1fYmY9gV0e9z/Iu+xNVSASWjsuyGU=
10881088
github.com/hdevalence/ed25519consensus v0.0.0-20220222234857-c00d1f31bab3/go.mod h1:5PC6ZNPde8bBqU/ewGZig35+UIZtw9Ytxez8/q5ZyFE=
1089-
github.com/hinshun/vt10x v0.0.0-20220119200601-820417d04eec h1:qv2VnGeEQHchGaZ/u7lxST/RaJw+cv273q79D81Xbog=
10901089
github.com/hinshun/vt10x v0.0.0-20220119200601-820417d04eec/go.mod h1:Q48J4R4DvxnHolD5P8pOtXigYlRuPLGl6moFx3ulM68=
10911090
github.com/hinshun/vt10x v0.0.0-20220301184237-5011da428d02 h1:AgcIVYPa6XJnU3phs104wLj8l5GEththEw6+F79YsIY=
10921091
github.com/hinshun/vt10x v0.0.0-20220301184237-5011da428d02/go.mod h1:Q48J4R4DvxnHolD5P8pOtXigYlRuPLGl6moFx3ulM68=

‎pty/pty_other.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,10 @@ import (
1010
"runtime"
1111
"sync"
1212

13-
"golang.org/x/xerrors"
14-
1513
"github.com/creack/pty"
1614
"github.com/u-root/u-root/pkg/termios"
1715
"golang.org/x/sys/unix"
16+
"golang.org/x/xerrors"
1817
)
1918

2019
func newPty(opt ...Option) (retPTY *otherPty, err error) {

‎pty/pty_windows.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -160,13 +160,15 @@ func (p *ptyWindows) Close() error {
160160
if p.outputWrite != nil {
161161
_ = p.outputWrite.Close()
162162
}
163-
if p.inputRead.Close() != nil {
163+
if p.inputRead != nil {
164164
_ = p.inputRead.Close()
165165
}
166166
return nil
167167
}
168168

169169
func (p *windowsProcess) waitInternal() {
170+
// put this on the bottom of the defer stack since the next defer can write to p.cmdErr
171+
defer close(p.cmdDone)
170172
defer func() {
171173
// close the pseudoconsole handle when the process exits, if it hasn't already been closed.
172174
// this is important because the PseudoConsole (conhost.exe) holds the write-end
@@ -186,7 +188,7 @@ func (p *windowsProcess) waitInternal() {
186188
p.pw.console = windows.InvalidHandle
187189
}
188190
}()
189-
defer close(p.cmdDone)
191+
190192
state, err := p.proc.Wait()
191193
if err != nil {
192194
p.cmdErr = err

‎pty/ptytest/ptytest.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ func New(t *testing.T, opts ...pty.Option) *PTY {
3232

3333
e := newExpecter(t, ptty.Output(), "cmd")
3434
r := &PTY{
35-
outExpecter: *e,
35+
outExpecter: e,
3636
PTY: ptty,
3737
}
3838
// Ensure pty is cleaned up at the end of test.
@@ -56,7 +56,7 @@ func Start(t *testing.T, cmd *exec.Cmd, opts ...pty.StartOption) (*PTYCmd, pty.P
5656
ex := newExpecter(t, ptty.OutputReader(), cmd.Args[0])
5757

5858
r := &PTYCmd{
59-
outExpecter: *ex,
59+
outExpecter: ex,
6060
PTYCmd: ptty,
6161
}
6262
t.Cleanup(func() {
@@ -65,7 +65,7 @@ func Start(t *testing.T, cmd *exec.Cmd, opts ...pty.StartOption) (*PTYCmd, pty.P
6565
return r, ps
6666
}
6767

68-
func newExpecter(t *testing.T, r io.Reader, name string) *outExpecter {
68+
func newExpecter(t *testing.T, r io.Reader, name string) outExpecter {
6969
// Use pipe for logging.
7070
logDone := make(chan struct{})
7171
logr, logw := io.Pipe()
@@ -75,7 +75,7 @@ func newExpecter(t *testing.T, r io.Reader, name string) *outExpecter {
7575
out := newStdbuf()
7676
w := io.MultiWriter(logw, out)
7777

78-
ex := &outExpecter{
78+
ex := outExpecter{
7979
t: t,
8080
out: out,
8181
name: name,

‎pty/start_test.go

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ func Test_Start_copy(t *testing.T) {
2929
pc, cmd, err := pty.Start(exec.CommandContext(ctx, cmdEcho, argEcho...))
3030
require.NoError(t, err)
3131
b := &bytes.Buffer{}
32-
readDone := make(chan error)
32+
readDone := make(chan error, 1)
3333
go func() {
3434
_, err := io.Copy(b, pc.OutputReader())
3535
readDone <- err
@@ -43,7 +43,7 @@ func Test_Start_copy(t *testing.T) {
4343
}
4444
assert.Contains(t, b.String(), "test")
4545

46-
cmdDone := make(chan error)
46+
cmdDone := make(chan error, 1)
4747
go func() {
4848
cmdDone <- cmd.Wait()
4949
}()
@@ -58,7 +58,7 @@ func Test_Start_copy(t *testing.T) {
5858

5959
// Test_Start_truncation tests that we can read command output without truncation
6060
// even after the command has exited.
61-
func Test_Start_trucation(t *testing.T) {
61+
func Test_Start_truncation(t *testing.T) {
6262
t.Parallel()
6363

6464
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitSuperLong)
@@ -94,7 +94,7 @@ func Test_Start_trucation(t *testing.T) {
9494
assert.NoError(t, err)
9595
}()
9696

97-
cmdDone := make(chan error)
97+
cmdDone := make(chan error, 1)
9898
go func() {
9999
cmdDone <- cmd.Wait()
100100
}()
@@ -103,14 +103,14 @@ func Test_Start_trucation(t *testing.T) {
103103
case err := <-cmdDone:
104104
require.NoError(t, err)
105105
case <-ctx.Done():
106-
t.Error("cmd.Wait() timed out")
106+
t.Fatal("cmd.Wait() timed out")
107107
}
108108

109109
select {
110110
case <-readDone:
111111
// OK!
112112
case <-ctx.Done():
113-
t.Error("read timed out")
113+
t.Fatal("read timed out")
114114
}
115115
}
116116

‎pty/start_windows.go

Lines changed: 27 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import (
1717

1818
// Allocates a PTY and starts the specified command attached to it.
1919
// See: https://docs.microsoft.com/en-us/windows/console/creating-a-pseudoconsole-session#creating-the-hosted-process
20-
func startPty(cmd *exec.Cmd, opt ...StartOption) (PTY, Process, error) {
20+
func startPty(cmd *exec.Cmd, opt ...StartOption) (_ PTY, _ Process, retErr error) {
2121
var opts startOptions
2222
for _, o := range opt {
2323
o(&opts)
@@ -45,10 +45,18 @@ func startPty(cmd *exec.Cmd, opt ...StartOption) (PTY, Process, error) {
4545
if err != nil {
4646
return nil, nil, err
4747
}
48+
4849
pty, err := newPty(opts.ptyOpts...)
4950
if err != nil {
5051
return nil, nil, err
5152
}
53+
defer func() {
54+
if retErr != nil {
55+
// we hit some error finishing setup; close pty, so
56+
// we don't leak the kernel resources associated with it
57+
_ := pty.Close()
58+
}
59+
}()
5260
winPty := pty.(*ptyWindows)
5361
if winPty.opts.sshReq != nil {
5462
cmd.Env = append(cmd.Env, fmt.Sprintf("SSH_TTY=%s", winPty.Name()))
@@ -87,6 +95,24 @@ func startPty(cmd *exec.Cmd, opt ...StartOption) (PTY, Process, error) {
8795
}
8896
defer windows.CloseHandle(processInfo.Thread)
8997
defer windows.CloseHandle(processInfo.Process)
98+
99+
process, err := os.FindProcess(int(processInfo.ProcessId))
100+
if err != nil {
101+
return nil, nil, xerrors.Errorf("find process %d: %w", processInfo.ProcessId, err)
102+
}
103+
wp := &windowsProcess{
104+
cmdDone: make(chan any),
105+
proc: process,
106+
pw: winPty,
107+
}
108+
defer func() {
109+
if retErr != nil {
110+
// if we later error out, kill the process since
111+
// the caller will have no way to interact with it
112+
_ = process.Kill()
113+
}
114+
}()
115+
90116
// Now that we've started the command, and passed the pseudoconsole to it,
91117
// close the output write and input read files, so that the other process
92118
// has the only handles to them. Once the process closes the console, there
@@ -103,16 +129,6 @@ func startPty(cmd *exec.Cmd, opt ...StartOption) (PTY, Process, error) {
103129
if errI != nil {
104130
return nil, nil, errI
105131
}
106-
107-
process, err := os.FindProcess(int(processInfo.ProcessId))
108-
if err != nil {
109-
return nil, nil, xerrors.Errorf("find process %d: %w", processInfo.ProcessId, err)
110-
}
111-
wp := &windowsProcess{
112-
cmdDone: make(chan any),
113-
proc: process,
114-
pw: winPty,
115-
}
116132
go wp.waitInternal()
117133
return pty, wp, nil
118134
}

0 commit comments

Comments
 (0)