daemon.ContainerStop(): fix for a negative timeout

1. As daemon.ContainerStop() documentation says,

> If a negative number of seconds is given, ContainerStop
> will wait for a graceful termination.

but since commit cfdf84d5d0 (PR #32237) this is no longer the case.

This happens because `context.WithTimeout(ctx, timeout)` is implemented
as `WithDeadline(ctx, time.Now().Add(timeout))`, resulting in a deadline
which is in the past.

To fix, don't use WithDeadline() if the timeout is negative.

2. Add a test case to validate the correct behavior and
as a means to prevent a similar regression in the future.

3. Fix/improve daemon.ContainerStop() and client.ContainerStop()
description for clarity and completeness.

4. Fix/improve DefaultStopTimeout description.

Fixes: cfdf84d5d0 ("Update Container Wait")
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

client/container_stop.go

@@ -8,8 +8,13 @@ import (
 	"golang.org/x/net/context"
 )
 
-// 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 {

container/container_unix.go

@@ -22,7 +22,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"

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)

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.IfCondition(t, testEnv.DaemonInfo.Driver != "devicemapper")