diff --git a/client/container_stop.go b/client/container_stop.go index 5d7f1606ec..629d7ab64c 100644 --- a/client/container_stop.go +++ b/client/container_stop.go @@ -8,8 +8,13 @@ import ( timetypes "github.com/docker/docker/api/types/time" ) -// ContainerStop stops a container without terminating the process. -// The process is blocked until the container stops or the timeout expires. +// ContainerStop stops a container. In case the container fails to stop +// gracefully within a time frame specified by the timeout argument, +// it is forcefully terminated (killed). +// +// If the timeout is nil, the container's StopTimeout value is used, if set, +// otherwise the engine default. A negative timeout value can be specified, +// meaning no timeout, i.e. no forceful termination is performed. func (cli *Client) ContainerStop(ctx context.Context, containerID string, timeout *time.Duration) error { query := url.Values{} if timeout != nil { diff --git a/container/container_unit_test.go b/container/container_unit_test.go index bf45df942e..fbee6e5eb0 100644 --- a/container/container_unit_test.go +++ b/container/container_unit_test.go @@ -52,9 +52,9 @@ func TestContainerStopTimeout(t *testing.T) { c = &Container{ Config: &container.Config{StopTimeout: &stopTimeout}, } - s = c.StopSignal() - if s != 15 { - t.Fatalf("Expected 15, got %v", s) + s = c.StopTimeout() + if s != stopTimeout { + t.Fatalf("Expected %v, got %v", stopTimeout, s) } } diff --git a/container/container_unix.go b/container/container_unix.go index c77ea07a18..597acf1218 100644 --- a/container/container_unix.go +++ b/container/container_unix.go @@ -23,7 +23,8 @@ import ( ) const ( - // DefaultStopTimeout is the timeout (in seconds) for the syscall signal used to stop a container. + // DefaultStopTimeout sets the default time, in seconds, to wait + // for the graceful container stop before forcefully terminating it. DefaultStopTimeout = 10 containerSecretMountPath = "/run/secrets" diff --git a/daemon/stop.go b/daemon/stop.go index 45f523848f..c3ac09056a 100644 --- a/daemon/stop.go +++ b/daemon/stop.go @@ -10,13 +10,15 @@ import ( "github.com/sirupsen/logrus" ) -// ContainerStop looks for the given container and terminates it, -// waiting the given number of seconds before forcefully killing the -// container. If a negative number of seconds is given, ContainerStop -// will wait for a graceful termination. An error is returned if the -// container is not found, is already stopped, or if there is a -// problem stopping the container. -func (daemon *Daemon) ContainerStop(name string, seconds *int) error { +// ContainerStop looks for the given container and stops it. +// In case the container fails to stop gracefully within a time duration +// specified by the timeout argument, in seconds, it is forcefully +// terminated (killed). +// +// If the timeout is nil, the container's StopTimeout value is used, if set, +// otherwise the engine default. A negative timeout value can be specified, +// meaning no timeout, i.e. no forceful termination is performed. +func (daemon *Daemon) ContainerStop(name string, timeout *int) error { container, err := daemon.GetContainer(name) if err != nil { return err @@ -24,21 +26,17 @@ func (daemon *Daemon) ContainerStop(name string, seconds *int) error { if !container.IsRunning() { return containerNotModifiedError{running: false} } - if seconds == nil { + if timeout == nil { stopTimeout := container.StopTimeout() - seconds = &stopTimeout + timeout = &stopTimeout } - if err := daemon.containerStop(container, *seconds); err != nil { + if err := daemon.containerStop(container, *timeout); err != nil { return errdefs.System(errors.Wrapf(err, "cannot stop container: %s", name)) } return nil } -// containerStop halts a container by sending a stop signal, waiting for the given -// duration in seconds, and then calling SIGKILL and waiting for the -// process to exit. If a negative duration is given, Stop will wait -// for the initial signal forever. If the container is not running Stop returns -// immediately. +// containerStop sends a stop signal, waits, sends a kill signal. func (daemon *Daemon) containerStop(container *containerpkg.Container, seconds int) error { if !container.IsRunning() { return nil @@ -69,8 +67,12 @@ func (daemon *Daemon) containerStop(container *containerpkg.Container, seconds i } // 2. Wait for the process to exit on its own - ctx, cancel := context.WithTimeout(context.Background(), time.Duration(seconds)*time.Second) - defer cancel() + ctx := context.Background() + if seconds >= 0 { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, time.Duration(seconds)*time.Second) + defer cancel() + } if status := <-container.Wait(ctx, containerpkg.WaitConditionNotRunning); status.Err() != nil { logrus.Infof("Container %v failed to exit within %d seconds of signal %d - using the force", container.ID, seconds, stopSignal) diff --git a/integration/container/stop_test.go b/integration/container/stop_test.go index c22a0302f6..2932ae0bf9 100644 --- a/integration/container/stop_test.go +++ b/integration/container/stop_test.go @@ -3,6 +3,7 @@ package container // import "github.com/docker/docker/integration/container" import ( "context" "fmt" + "strconv" "strings" "testing" "time" @@ -42,6 +43,60 @@ func TestStopContainerWithRestartPolicyAlways(t *testing.T) { } } +// TestStopContainerWithTimeout checks that ContainerStop with +// a timeout works as documented, i.e. in case of negative timeout +// waiting is not limited (issue #35311). +func TestStopContainerWithTimeout(t *testing.T) { + defer setupTest(t)() + client := request.NewAPIClient(t) + ctx := context.Background() + + testCmd := container.WithCmd("sh", "-c", "sleep 2 && exit 42") + testData := []struct { + doc string + timeout int + expectedExitCode int + }{ + // In case container is forcefully killed, 137 is returned, + // otherwise the exit code from the above script + { + "zero timeout: expect forceful container kill", + 0, 137, + }, + { + "too small timeout: expect forceful container kill", + 1, 137, + }, + { + "big enough timeout: expect graceful container stop", + 3, 42, + }, + { + "unlimited timeout: expect graceful container stop", + -1, 42, + }, + } + + for _, d := range testData { + d := d + t.Run(strconv.Itoa(d.timeout), func(t *testing.T) { + t.Parallel() + id := container.Run(t, ctx, client, testCmd) + + timeout := time.Duration(d.timeout) * time.Second + err := client.ContainerStop(ctx, id, &timeout) + assert.NilError(t, err) + + poll.WaitOn(t, container.IsStopped(ctx, client, id), + poll.WithDelay(100*time.Millisecond)) + + inspect, err := client.ContainerInspect(ctx, id) + assert.NilError(t, err) + assert.Equal(t, inspect.State.ExitCode, d.expectedExitCode) + }) + } +} + func TestDeleteDevicemapper(t *testing.T) { skip.If(t, testEnv.DaemonInfo.Driver != "devicemapper") skip.If(t, testEnv.IsRemoteDaemon, "cannot start daemon on remote test run")