From 35787ec6ecbb72424859706504deff5890c85056 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 2 Dec 2022 11:45:34 +0000 Subject: [PATCH 1/4] fix: Close ptytest ttys asynchronously to enable timeout --- pty/ptytest/ptytest.go | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/pty/ptytest/ptytest.go b/pty/ptytest/ptytest.go index 635176a155a67..5721dfc346848 100644 --- a/pty/ptytest/ptytest.go +++ b/pty/ptytest/ptytest.go @@ -72,11 +72,29 @@ func create(t *testing.T, ptty pty.PTY, name string) *PTY { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) defer cancel() - _ = out.Close() - _ = ptty.Close() + errC := make(chan error, 2) + doClose := func(c io.Closer) { + select { + case <-ctx.Done(): + case errC <- c.Close(): + } + } + // Close the tty asynchronously to allow the select below to timeout. + go func() { + // Close the tty first so allow out to consume its last bytes. + doClose(ptty) + doClose(out) + }() + + More: select { case <-ctx.Done(): fatalf(t, name, "cleanup", "copy did not close in time") + case err := <-errC: + if err != nil { + logf(t, name, "copy close error: %v", err) + } + goto More case <-copyDone: } }) From af76f94a644583a34184edeb6399e055fa07bb82 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 2 Dec 2022 12:02:31 +0000 Subject: [PATCH 2/4] Log what we are closing in ptytest --- .vscode/settings.json | 1 + pty/ptytest/ptytest.go | 22 ++++++++++++++-------- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index 4a0ab16e558ed..292ee5cd7c4ad 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -107,6 +107,7 @@ "slogtest", "sourcemapped", "Srcs", + "stdbuf", "stretchr", "STTY", "stuntest", diff --git a/pty/ptytest/ptytest.go b/pty/ptytest/ptytest.go index 5721dfc346848..2504702e8bf1e 100644 --- a/pty/ptytest/ptytest.go +++ b/pty/ptytest/ptytest.go @@ -72,27 +72,33 @@ func create(t *testing.T, ptty pty.PTY, name string) *PTY { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) defer cancel() - errC := make(chan error, 2) - doClose := func(c io.Closer) { + type message struct { + name string + err error + } + closeC := make(chan message, 2) + doClose := func(name string, c io.Closer) { select { case <-ctx.Done(): - case errC <- c.Close(): + case closeC <- message{name: name, err: c.Close()}: } } // Close the tty asynchronously to allow the select below to timeout. go func() { // Close the tty first so allow out to consume its last bytes. - doClose(ptty) - doClose(out) + doClose("ptty", ptty) + doClose("stdbuf", out) }() More: select { case <-ctx.Done(): fatalf(t, name, "cleanup", "copy did not close in time") - case err := <-errC: - if err != nil { - logf(t, name, "copy close error: %v", err) + case m := <-closeC: + if m.err != nil { + logf(t, name, "copy close error: %s: %v", m.name, m.err) + } else { + logf(t, name, "copy closed: %s", m.name) } goto More case <-copyDone: From ba7d55a2c996642aa311dd4df3a8a60525ca20a7 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 2 Dec 2022 12:06:50 +0000 Subject: [PATCH 3/4] Simplify --- pty/ptytest/ptytest.go | 30 +++++------------------------- 1 file changed, 5 insertions(+), 25 deletions(-) diff --git a/pty/ptytest/ptytest.go b/pty/ptytest/ptytest.go index 2504702e8bf1e..4ffa94ca5a203 100644 --- a/pty/ptytest/ptytest.go +++ b/pty/ptytest/ptytest.go @@ -14,6 +14,7 @@ import ( "time" "unicode/utf8" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/xerrors" @@ -72,35 +73,14 @@ func create(t *testing.T, ptty pty.PTY, name string) *PTY { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort) defer cancel() - type message struct { - name string - err error - } - closeC := make(chan message, 2) - doClose := func(name string, c io.Closer) { - select { - case <-ctx.Done(): - case closeC <- message{name: name, err: c.Close()}: - } - } - // Close the tty asynchronously to allow the select below to timeout. - go func() { - // Close the tty first so allow out to consume its last bytes. - doClose("ptty", ptty) - doClose("stdbuf", out) - }() + // Close pty only so that the copy goroutine can consume the + // remainder of it's buffer and then exit. + err := ptty.Close() + assert.NoError(t, err, "close pty") - More: select { case <-ctx.Done(): fatalf(t, name, "cleanup", "copy did not close in time") - case m := <-closeC: - if m.err != nil { - logf(t, name, "copy close error: %s: %v", m.name, m.err) - } else { - logf(t, name, "copy closed: %s", m.name) - } - goto More case <-copyDone: } }) From 5147c22afaaabb6024637fec5889b045e9437632 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 2 Dec 2022 12:14:21 +0000 Subject: [PATCH 4/4] Avoid failing test --- pty/ptytest/ptytest.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pty/ptytest/ptytest.go b/pty/ptytest/ptytest.go index 4ffa94ca5a203..bc26585f2962a 100644 --- a/pty/ptytest/ptytest.go +++ b/pty/ptytest/ptytest.go @@ -14,7 +14,6 @@ import ( "time" "unicode/utf8" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/xerrors" @@ -76,7 +75,9 @@ func create(t *testing.T, ptty pty.PTY, name string) *PTY { // Close pty only so that the copy goroutine can consume the // remainder of it's buffer and then exit. err := ptty.Close() - assert.NoError(t, err, "close pty") + // Pty may already be closed, so don't fail the test, but log + // the error in case it's significant. + logf(t, name, "closed pty: %v", err) select { case <-ctx.Done():