From b811057181adb74baa05a5ad2ac47d7caad60803 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= <pawel.gronowski@docker.com>
Date: Wed, 4 Jan 2023 10:47:46 +0100
Subject: [PATCH] cli/rm_test: Fix TestRemoveForce race condition
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Synchronize append on the `removed` slice with mutex because
containerRemoveFunc is called in parallel for each removed container by
`container rm` cli command.
Also reduced the shared access area by separating the scopes of test
cases.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
---
 cli/command/container/rm_test.go | 72 ++++++++++++++++++--------------
 1 file changed, 41 insertions(+), 31 deletions(-)

diff --git a/cli/command/container/rm_test.go b/cli/command/container/rm_test.go
index 24e621f516..7ab83dd2c9 100644
--- a/cli/command/container/rm_test.go
+++ b/cli/command/container/rm_test.go
@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"io"
 	"sort"
+	"sync"
 	"testing"
 
 	"github.com/docker/cli/internal/test"
@@ -14,37 +15,46 @@ import (
 )
 
 func TestRemoveForce(t *testing.T) {
-	var (
-		removed1 []string
-		removed2 []string
-	)
+	for _, tc := range []struct {
+		name        string
+		args        []string
+		expectedErr string
+	}{
+		{name: "without force", args: []string{"nosuchcontainer", "mycontainer"}, expectedErr: "no such container"},
+		{name: "with force", args: []string{"--force", "nosuchcontainer", "mycontainer"}, expectedErr: ""},
+	} {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			var removed []string
+			mutex := new(sync.Mutex)
 
-	cli := test.NewFakeCli(&fakeClient{
-		containerRemoveFunc: func(ctx context.Context, container string, options types.ContainerRemoveOptions) error {
-			removed1 = append(removed1, container)
-			removed2 = append(removed2, container)
-			if container == "nosuchcontainer" {
-				return errdefs.NotFound(fmt.Errorf("Error: no such container: " + container))
+			cli := test.NewFakeCli(&fakeClient{
+				containerRemoveFunc: func(ctx context.Context, container string, options types.ContainerRemoveOptions) error {
+					// containerRemoveFunc is called in parallel for each container
+					// by the remove command so append must be synchronized.
+					mutex.Lock()
+					removed = append(removed, container)
+					mutex.Unlock()
+
+					if container == "nosuchcontainer" {
+						return errdefs.NotFound(fmt.Errorf("Error: no such container: " + container))
+					}
+					return nil
+				},
+				Version: "1.36",
+			})
+			cmd := NewRmCommand(cli)
+			cmd.SetOut(io.Discard)
+			cmd.SetArgs(tc.args)
+
+			err := cmd.Execute()
+			if tc.expectedErr != "" {
+				assert.ErrorContains(t, err, tc.expectedErr)
+			} else {
+				assert.NilError(t, err)
 			}
-			return nil
-		},
-		Version: "1.36",
-	})
-	cmd := NewRmCommand(cli)
-	cmd.SetOut(io.Discard)
-
-	t.Run("without force", func(t *testing.T) {
-		cmd.SetArgs([]string{"nosuchcontainer", "mycontainer"})
-		removed1 = []string{}
-		assert.ErrorContains(t, cmd.Execute(), "no such container")
-		sort.Strings(removed1)
-		assert.DeepEqual(t, removed1, []string{"mycontainer", "nosuchcontainer"})
-	})
-	t.Run("with force", func(t *testing.T) {
-		cmd.SetArgs([]string{"--force", "nosuchcontainer", "mycontainer"})
-		removed2 = []string{}
-		assert.NilError(t, cmd.Execute())
-		sort.Strings(removed2)
-		assert.DeepEqual(t, removed2, []string{"mycontainer", "nosuchcontainer"})
-	})
+			sort.Strings(removed)
+			assert.DeepEqual(t, removed, []string{"mycontainer", "nosuchcontainer"})
+		})
+	}
 }