From d0847c2dce2fd0304c047acb88eebab990094bd6 Mon Sep 17 00:00:00 2001 From: Florent Poinsard <35779988+frouioui@users.noreply.github.com> Date: Mon, 18 Sep 2023 09:45:17 -0400 Subject: [PATCH] Enable failures in `tools/e2e_test_race.sh` and fix races (#13654) Signed-off-by: Florent Poinsard --- go/vt/vttablet/endtoend/framework/client.go | 6 ++++++ go/vt/vttablet/endtoend/transaction_test.go | 12 ++++++------ go/vt/vttablet/tabletserver/throttle/client.go | 5 ++++- tools/e2e_test_race.sh | 4 +++- 4 files changed, 19 insertions(+), 8 deletions(-) diff --git a/go/vt/vttablet/endtoend/framework/client.go b/go/vt/vttablet/endtoend/framework/client.go index 4d5d8bd6a18..3c06f9b465c 100644 --- a/go/vt/vttablet/endtoend/framework/client.go +++ b/go/vt/vttablet/endtoend/framework/client.go @@ -19,6 +19,7 @@ package framework import ( "context" "errors" + "sync" "time" "vitess.io/vitess/go/sqltypes" @@ -38,6 +39,7 @@ type QueryClient struct { target *querypb.Target server *tabletserver.TabletServer transactionID int64 + reservedIDMu sync.Mutex reservedID int64 sessionStateChanges string } @@ -112,6 +114,8 @@ func (client *QueryClient) Commit() error { func (client *QueryClient) Rollback() error { defer func() { client.transactionID = 0 }() rID, err := client.server.Rollback(client.ctx, client.target, client.transactionID) + client.reservedIDMu.Lock() + defer client.reservedIDMu.Unlock() client.reservedID = rID if err != nil { return err @@ -291,6 +295,8 @@ func (client *QueryClient) MessageAck(name string, ids []string) (int64, error) // ReserveExecute performs a ReserveExecute. func (client *QueryClient) ReserveExecute(query string, preQueries []string, bindvars map[string]*querypb.BindVariable) (*sqltypes.Result, error) { + client.reservedIDMu.Lock() + defer client.reservedIDMu.Unlock() if client.reservedID != 0 { return nil, errors.New("already reserved a connection") } diff --git a/go/vt/vttablet/endtoend/transaction_test.go b/go/vt/vttablet/endtoend/transaction_test.go index 6751e60f9ad..8f6546df5f1 100644 --- a/go/vt/vttablet/endtoend/transaction_test.go +++ b/go/vt/vttablet/endtoend/transaction_test.go @@ -321,7 +321,7 @@ func TestShutdownGracePeriod(t *testing.T) { err := client.Begin(false) require.NoError(t, err) go func() { - _, err = client.Execute("select sleep(10) from dual", nil) + _, err := client.Execute("select sleep(10) from dual", nil) assert.Error(t, err) }() @@ -346,7 +346,7 @@ func TestShutdownGracePeriod(t *testing.T) { err = client.Begin(false) require.NoError(t, err) go func() { - _, err = client.Execute("select sleep(11) from dual", nil) + _, err := client.Execute("select sleep(11) from dual", nil) assert.Error(t, err) }() @@ -373,7 +373,7 @@ func TestShutdownGracePeriodWithStreamExecute(t *testing.T) { err := client.Begin(false) require.NoError(t, err) go func() { - _, err = client.StreamExecute("select sleep(10) from dual", nil) + _, err := client.StreamExecute("select sleep(10) from dual", nil) assert.Error(t, err) }() @@ -398,7 +398,7 @@ func TestShutdownGracePeriodWithStreamExecute(t *testing.T) { err = client.Begin(false) require.NoError(t, err) go func() { - _, err = client.StreamExecute("select sleep(11) from dual", nil) + _, err := client.StreamExecute("select sleep(11) from dual", nil) assert.Error(t, err) }() @@ -425,7 +425,7 @@ func TestShutdownGracePeriodWithReserveExecute(t *testing.T) { err := client.Begin(false) require.NoError(t, err) go func() { - _, err = client.ReserveExecute("select sleep(10) from dual", nil, nil) + _, err := client.ReserveExecute("select sleep(10) from dual", nil, nil) assert.Error(t, err) }() @@ -450,7 +450,7 @@ func TestShutdownGracePeriodWithReserveExecute(t *testing.T) { err = client.Begin(false) require.NoError(t, err) go func() { - _, err = client.ReserveExecute("select sleep(11) from dual", nil, nil) + _, err := client.ReserveExecute("select sleep(11) from dual", nil, nil) assert.Error(t, err) }() diff --git a/go/vt/vttablet/tabletserver/throttle/client.go b/go/vt/vttablet/tabletserver/throttle/client.go index 69bf4bac0cb..41888340b5a 100644 --- a/go/vt/vttablet/tabletserver/throttle/client.go +++ b/go/vt/vttablet/tabletserver/throttle/client.go @@ -52,7 +52,8 @@ type Client struct { checkType ThrottleCheckType flags CheckFlags - lastSuccessfulThrottle int64 + lastSuccessfulThrottleMu sync.Mutex + lastSuccessfulThrottle int64 } // NewProductionClient creates a client suitable for foreground/production jobs, which have normal priority. @@ -96,6 +97,8 @@ func (c *Client) ThrottleCheckOK(ctx context.Context, overrideAppName throttlera // no throttler return true } + c.lastSuccessfulThrottleMu.Lock() + defer c.lastSuccessfulThrottleMu.Unlock() if c.lastSuccessfulThrottle >= atomic.LoadInt64(&throttleTicks) { // if last check was OK just very recently there is no need to check again return true diff --git a/tools/e2e_test_race.sh b/tools/e2e_test_race.sh index 7dad5b259a3..b072e1261e2 100755 --- a/tools/e2e_test_race.sh +++ b/tools/e2e_test_race.sh @@ -38,9 +38,11 @@ packages_with_tests=$(echo "$packages_with_tests" | grep -vE "go/test/endtoend" # endtoend tests should be in a directory called endtoend all_e2e_tests=$(echo "$packages_with_tests" | cut -d" " -f1) +set -exo pipefail + # Run all endtoend tests. echo "$all_e2e_tests" | xargs go test $VT_GO_PARALLEL -race 2>&1 | tee $temp_log_file -if [ ${PIPESTATUS[0]} -ne 0 ]; then +if [ ${PIPESTATUS[1]} -ne 0 ]; then if grep "WARNING: DATA RACE" -q $temp_log_file; then echo echo "ERROR: go test -race found a data race. See log above."