Skip to content

Commit fc94ed0

Browse files
thaJeztahthaJeztah
committed
don't cancel container stop when cancelling context
Commit 90de570 passed through the request context to daemon.ContainerStop(). As a result, cancelling the context would cancel the "graceful" stop of the container, and would proceed with forcefully killing the container. This patch partially reverts the changes from 90de570 and breaks the context to prevent cancelling the context from cancelling the stop. Signed-off-by: Sebastiaan van Stijn <[email protected]> Signed-off-by: Sebastiaan van Stijn <[email protected]>
1 parent 3fae7ea commit fc94ed0

2 files changed

Lines changed: 89 additions & 1 deletion

File tree

‎daemon/stop.go‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,13 @@ func (daemon *Daemon) ContainerStop(ctx context.Context, name string, options co
3636
}
3737

3838
// containerStop sends a stop signal, waits, sends a kill signal.
39-
func (daemon *Daemon) containerStop(ctx context.Context, ctr *container.Container, options containertypes.StopOptions) (retErr error) {
39+
func (daemon *Daemon) containerStop(_ context.Context, ctr *container.Container, options containertypes.StopOptions) (retErr error) {
40+
// Deliberately using a local context here, because cancelling the
41+
// request should not cancel the stop.
42+
//
43+
// TODO(thaJeztah): pass context, and use context.WithoutCancel() once available: https://github.com/golang/go/issues/40221
44+
ctx := context.Background()
45+
4046
if !ctr.IsRunning() {
4147
return nil
4248
}

‎integration/container/stop_linux_test.go‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,22 @@
11
package container // import "github.com/docker/docker/integration/container"
22

33
import (
4+
"bytes"
45
"context"
6+
"io"
57
"strconv"
8+
"strings"
69
"testing"
710
"time"
811

12+
"github.com/docker/docker/api/types"
913
containertypes "github.com/docker/docker/api/types/container"
14+
"github.com/docker/docker/client"
15+
"github.com/docker/docker/errdefs"
1016
"github.com/docker/docker/integration/internal/container"
17+
"github.com/docker/docker/pkg/stdcopy"
1118
"gotest.tools/v3/assert"
19+
is "gotest.tools/v3/assert/cmp"
1220
"gotest.tools/v3/poll"
1321
)
1422

@@ -64,3 +72,77 @@ func TestStopContainerWithTimeout(t *testing.T) {
6472
})
6573
}
6674
}
75+
76+
// TestStopContainerWithTimeoutCancel checks that ContainerStop is not cancelled
77+
// if the request is cancelled.
78+
// See issue https://github.com/moby/moby/issues/45731
79+
func TestStopContainerWithTimeoutCancel(t *testing.T) {
80+
t.Parallel()
81+
defer setupTest(t)()
82+
apiClient := testEnv.APIClient()
83+
t.Cleanup(func() { _ = apiClient.Close() })
84+
85+
ctx := context.Background()
86+
id := container.Run(ctx, t, apiClient,
87+
container.WithCmd("sh", "-c", "trap 'echo received TERM' TERM; while true; do usleep 10; done"),
88+
)
89+
poll.WaitOn(t, container.IsInState(ctx, apiClient, id, "running"))
90+
91+
ctxCancel, cancel := context.WithCancel(context.Background())
92+
t.Cleanup(cancel)
93+
const stopTimeout = 3
94+
95+
stoppedCh := make(chan error)
96+
go func() {
97+
sto := stopTimeout
98+
stoppedCh <- apiClient.ContainerStop(ctxCancel, id, containertypes.StopOptions{Timeout: &sto})
99+
}()
100+
101+
poll.WaitOn(t, logsContains(ctx, apiClient, id, "received TERM"))
102+
103+
// Cancel the context once we verified the container was signaled, and check
104+
// that the container is not killed immediately
105+
cancel()
106+
107+
select {
108+
case stoppedErr := <-stoppedCh:
109+
assert.Check(t, is.ErrorType(stoppedErr, errdefs.IsCancelled))
110+
case <-time.After(5 * time.Second):
111+
t.Fatal("timeout waiting for stop request to be cancelled")
112+
}
113+
inspect, err := apiClient.ContainerInspect(ctx, id)
114+
assert.Check(t, err)
115+
assert.Check(t, inspect.State.Running)
116+
117+
// container should be stopped after stopTimeout is reached. The daemon.containerStop
118+
// code is rather convoluted, and waits another 2 seconds for the container to
119+
// terminate after signaling it;
120+
// https://github.com/moby/moby/blob/97455cc31ffa08078db6591f018256ed59c35bbc/daemon/stop.go#L101-L112
121+
//
122+
// Adding 3 seconds to the specified stopTimeout to take this into account,
123+
// and add another second margin to try to avoid flakiness.
124+
poll.WaitOn(t, container.IsStopped(ctx, apiClient, id), poll.WithTimeout((3+stopTimeout)*time.Second))
125+
}
126+
127+
// logsContains verifies the container contains the given text in the log's stdout.
128+
func logsContains(ctx context.Context, client client.APIClient, containerID string, logString string) func(log poll.LogT) poll.Result {
129+
return func(log poll.LogT) poll.Result {
130+
logs, err := client.ContainerLogs(ctx, containerID, types.ContainerLogsOptions{
131+
ShowStdout: true,
132+
})
133+
if err != nil {
134+
return poll.Error(err)
135+
}
136+
defer logs.Close()
137+
138+
var stdout bytes.Buffer
139+
_, err = stdcopy.StdCopy(&stdout, io.Discard, logs)
140+
if err != nil {
141+
return poll.Error(err)
142+
}
143+
if strings.Contains(stdout.String(), logString) {
144+
return poll.Success()
145+
}
146+
return poll.Continue("waiting for logstring '%s' in container", logString)
147+
}
148+
}

0 commit comments

Comments
 (0)