Skip to content

Commit 9a57e74

Browse files
invidiancpuguy83
authored andcommitted
test/testenv: let Docker CLI shut down dial-stdio gracefully
After looking into buildx and Docker CLI plugin runtime implementation, eventually I got to this code, which seems to be properly cleaning up the dial-stdio process when test suite gets interrupted or panics, without using process group and threads locking, which are OS specific. Integration tests now explicitly close client connection which makes dial-stdio process to exit on its own gracefully, even without passing the interrupt signal, which I think is an elegant solution. Closes #974 Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>
1 parent ba7d383 commit 9a57e74

File tree

2 files changed

+39
-23
lines changed

2 files changed

+39
-23
lines changed

test/main_test.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,12 @@ func TestMain(m *testing.M) {
9696
cancel()
9797
}()
9898

99+
defer func() {
100+
if err := testEnv.Close(); err != nil {
101+
fmt.Fprintln(os.Stderr, "Error closing test environment:", err)
102+
}
103+
}()
104+
99105
if err := testEnv.Load(ctx, phonyRef, fixtures.PhonyFrontend); err != nil {
100106
panic(err)
101107
}

test/testenv/buildx.go

Lines changed: 33 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,20 @@ func (b *BuildxEnv) WithBuilder(builder string) *BuildxEnv {
5555
return b
5656
}
5757

58+
// Close closes the underlying buildkit client connection, which triggers
59+
// cleanup of the dial-stdio process.
60+
func (b *BuildxEnv) Close() error {
61+
b.mu.Lock()
62+
defer b.mu.Unlock()
63+
64+
if b.client != nil {
65+
err := b.client.Close()
66+
b.client = nil
67+
return err
68+
}
69+
return nil
70+
}
71+
5872
// Load loads the output of the specified [gwclient.BuildFunc] into the buildkit instance.
5973
func (b *BuildxEnv) Load(ctx context.Context, id string, f gwclient.BuildFunc) error {
6074
if b.refs == nil {
@@ -86,10 +100,7 @@ func (c *connCloseWrapper) Close() error {
86100
if c.close != nil {
87101
c.close()
88102
}
89-
if err := c.Conn.Close(); err != nil {
90-
return err
91-
}
92-
return nil
103+
return c.Conn.Close()
93104
}
94105

95106
func (b *BuildxEnv) dialStdio(ctx context.Context) error {
@@ -116,9 +127,9 @@ func (b *BuildxEnv) dialStdio(ctx context.Context) error {
116127
cmd := exec.Command("docker", args...)
117128
cmd.Env = os.Environ()
118129

119-
c1, c2 := net.Pipe()
120-
cmd.Stdin = c1
121-
cmd.Stdout = c1
130+
dialStdioConn, clientConn := net.Pipe()
131+
cmd.Stdin = dialStdioConn
132+
cmd.Stdout = dialStdioConn
122133

123134
// Use a pipe to check when the connection is actually complete
124135
// Also write all of stderr to an error buffer so we can have more details
@@ -132,12 +143,15 @@ func (b *BuildxEnv) dialStdio(ctx context.Context) error {
132143
return nil, err
133144
}
134145

146+
// chWait is closed when cmd.Wait() returns, signaling the cleanup
147+
// function that the process has exited.
135148
chWait := make(chan struct{})
136149
go func() {
137150
err := cmd.Wait()
138-
c1.Close()
151+
close(chWait)
152+
dialStdioConn.Close()
139153
// pkgerrors.Wrap will return nil if err is nil, otherwise it will give
140-
// us a wrapped error with the buffered stderr from he command.
154+
// us a wrapped error with the buffered stderr from the command.
141155
w.CloseWithError(pkgerrors.Wrapf(err, "%s", errBuf))
142156
}()
143157

@@ -160,28 +174,26 @@ func (b *BuildxEnv) dialStdio(ctx context.Context) error {
160174
}
161175

162176
out := &connCloseWrapper{
163-
Conn: c2,
177+
Conn: clientConn,
164178
close: sync.OnceFunc(func() {
165-
// Send 2 interrupt signals to the process to ensure it exits gracefully
166-
// This is how buildx/docker plugins handle termination
167-
168-
cmd.Process.Signal(os.Interrupt) //nolint:errcheck // We don't care about this error, we are going to send another one anyway
169-
if err := cmd.Process.Signal(os.Interrupt); err != nil {
170-
cmd.Process.Kill() //nolint:errcheck // Force kill if interrupt fails
171-
}
179+
// Close the stdin/stdout pipe to the process.
180+
// This causes stdin EOF in buildx's dial-stdio, which triggers
181+
// closeWrite(conn) on the buildkit connection and should start
182+
// the chain reaction for docker CLI process to exit.
183+
dialStdioConn.Close()
172184

173185
select {
174186
case <-chWait:
175187
case <-time.After(10 * time.Second):
176-
// If it still doesn't exit, force kill
177-
cmd.Process.Kill() //nolint:errcheck // Force kill if it doesn't exit after interrupt
188+
// Safety net: force kill if still running.
189+
cmd.Process.Kill() //nolint:errcheck
190+
<-chWait
178191
}
179192
}),
180193
}
181194

182195
return out, nil
183196
}))
184-
185197
if err != nil {
186198
return err
187199
}
@@ -342,9 +354,7 @@ func (b *BuildxEnv) RunTest(ctx context.Context, t *testing.T, f TestFunc, opts
342354
t.Fatalf("%+v", err)
343355
}
344356

345-
var (
346-
ch chan *client.SolveStatus
347-
)
357+
var ch chan *client.SolveStatus
348358

349359
if cfg.SolveStatusFn != nil {
350360
ch = make(chan *client.SolveStatus, 1)

0 commit comments

Comments
 (0)