From 58ff83f72e7cd5743d0677f7c2b43b06e324987d Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Wed, 19 Jun 2024 13:33:21 +0300 Subject: [PATCH 01/37] add context with timeout check to vet script --- scripts/vet.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/scripts/vet.sh b/scripts/vet.sh index f88529e7d146..7dac4c9ffdd5 100755 --- a/scripts/vet.sh +++ b/scripts/vet.sh @@ -50,6 +50,9 @@ not grep 'func Test[^(]' -- test/*.go git grep 'func (s) ' -- "*_test.go" | not grep -v 'func (s) Test' git grep 'func [A-Z]' -- "*_test.go" | not grep -v 'func Test\|Benchmark\|Example' +# - Make sure not context usage are done without timeout. +git grep 'context.Background()' -- "*_test.go" | not grep -v 'context.WithTimeout(context.Background()' + # - Do not use time.After except in tests. It has the potential to leak the # timer since there is no way to stop it early. git grep -l 'time.After(' -- "*.go" | not grep -v '_test.go\|test_utils\|testutils' From 7da792013296a552bfc442bc537994c68bd21a06 Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Wed, 19 Jun 2024 14:12:45 +0300 Subject: [PATCH 02/37] expand lint rule to avoid dealing with context.WithCancel cases --- scripts/vet.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/vet.sh b/scripts/vet.sh index 7dac4c9ffdd5..32f3d02f03af 100755 --- a/scripts/vet.sh +++ b/scripts/vet.sh @@ -50,8 +50,8 @@ not grep 'func Test[^(]' -- test/*.go git grep 'func (s) ' -- "*_test.go" | not grep -v 'func (s) Test' git grep 'func [A-Z]' -- "*_test.go" | not grep -v 'func Test\|Benchmark\|Example' -# - Make sure not context usage are done without timeout. -git grep 'context.Background()' -- "*_test.go" | not grep -v 'context.WithTimeout(context.Background()' +# - Make sure all context usages are done timeout. +git grep -e 'context.Background()' --or -e 'context.TODO()' -- "*_test.go" | not grep -v 'context.WithTimeout(context.Background()' | not grep -v 'context.WithCancel(context.Background' | not grep -v 'context.WithTimeOut(context.TODO()' # - Do not use time.After except in tests. It has the potential to leak the # timer since there is no way to stop it early. From d47051ecac381c6bb0aef100053c69030935014b Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Wed, 19 Jun 2024 14:19:10 +0300 Subject: [PATCH 03/37] add context.WithTimeout to handeshaker tests --- credentials/alts/internal/handshaker/handshaker_test.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/credentials/alts/internal/handshaker/handshaker_test.go b/credentials/alts/internal/handshaker/handshaker_test.go index 9f877c48dcf3..770daf072bb7 100644 --- a/credentials/alts/internal/handshaker/handshaker_test.go +++ b/credentials/alts/internal/handshaker/handshaker_test.go @@ -309,7 +309,9 @@ func (s) TestNewClientHandshaker(t *testing.T) { conn := testutil.NewTestConn(nil, nil) clientConn := &grpc.ClientConn{} opts := &ClientHandshakerOptions{} - hs, err := NewClientHandshaker(context.Background(), clientConn, conn, opts) + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + hs, err := NewClientHandshaker(ctx, clientConn, conn, opts) if err != nil { t.Errorf("NewClientHandshaker returned unexpected error: %v", err) } @@ -341,7 +343,9 @@ func (s) TestNewServerHandshaker(t *testing.T) { conn := testutil.NewTestConn(nil, nil) clientConn := &grpc.ClientConn{} opts := &ServerHandshakerOptions{} - hs, err := NewServerHandshaker(context.Background(), clientConn, conn, opts) + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + hs, err := NewServerHandshaker(ctx, clientConn, conn, opts) if err != nil { t.Errorf("NewServerHandshaker returned unexpected error: %v", err) } From 5acc6e3c29c81680d72aa2022b541bd0fe4d5bcc Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Wed, 19 Jun 2024 16:22:41 +0300 Subject: [PATCH 04/37] avoid context_test.go in linting --- scripts/vet.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/vet.sh b/scripts/vet.sh index 32f3d02f03af..06a25eb0b003 100755 --- a/scripts/vet.sh +++ b/scripts/vet.sh @@ -51,7 +51,7 @@ git grep 'func (s) ' -- "*_test.go" | not grep -v 'func (s) Test' git grep 'func [A-Z]' -- "*_test.go" | not grep -v 'func Test\|Benchmark\|Example' # - Make sure all context usages are done timeout. -git grep -e 'context.Background()' --or -e 'context.TODO()' -- "*_test.go" | not grep -v 'context.WithTimeout(context.Background()' | not grep -v 'context.WithCancel(context.Background' | not grep -v 'context.WithTimeOut(context.TODO()' +git grep -e 'context.Background()' --or -e 'context.TODO()' -- "*_test.go" ! "benchmark/primitives/context_test.go" | not grep -v 'context.WithTimeout(context.Background()' | not grep -v 'context.WithTimeOut(context.TODO()' # - Do not use time.After except in tests. It has the potential to leak the # timer since there is no way to stop it early. From c47300f4935fcf88f42d8239bdf47b9be9cbb98d Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Wed, 19 Jun 2024 16:47:36 +0300 Subject: [PATCH 05/37] Change the lint rule to avoid context_test.go file --- scripts/vet.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/vet.sh b/scripts/vet.sh index 06a25eb0b003..43081b0bac6f 100755 --- a/scripts/vet.sh +++ b/scripts/vet.sh @@ -51,7 +51,7 @@ git grep 'func (s) ' -- "*_test.go" | not grep -v 'func (s) Test' git grep 'func [A-Z]' -- "*_test.go" | not grep -v 'func Test\|Benchmark\|Example' # - Make sure all context usages are done timeout. -git grep -e 'context.Background()' --or -e 'context.TODO()' -- "*_test.go" ! "benchmark/primitives/context_test.go" | not grep -v 'context.WithTimeout(context.Background()' | not grep -v 'context.WithTimeOut(context.TODO()' +git grep -e 'context.Background()' --or -e 'context.TODO()' -- "*_test.go" | grep -v "benchmark/primitives/context_test.go" | not grep -v 'context.WithTimeout(context.Background()' | not grep -v 'context.WithTimeOut(context.TODO()' # - Do not use time.After except in tests. It has the potential to leak the # timer since there is no way to stop it early. From d59584242072c272659092dc5fa3a0e3294146dd Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Wed, 19 Jun 2024 17:48:58 +0300 Subject: [PATCH 06/37] add context with timeout to observability tests --- gcp/observability/observability_test.go | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/gcp/observability/observability_test.go b/gcp/observability/observability_test.go index 28e8b1ae6992..e7da642acee6 100644 --- a/gcp/observability/observability_test.go +++ b/gcp/observability/observability_test.go @@ -184,7 +184,9 @@ func (s) TestRefuseStartWithInvalidPatterns(t *testing.T) { envconfig.ObservabilityConfigFile = oldObservabilityConfigFile }() // If there is at least one invalid pattern, which should not be silently tolerated. - if err := Start(context.Background()); err == nil { + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + if err := Start(ctx); err == nil { t.Fatalf("Invalid patterns not triggering error") } } @@ -220,7 +222,9 @@ func (s) TestRefuseStartWithExcludeAndWildCardAll(t *testing.T) { envconfig.ObservabilityConfigFile = oldObservabilityConfigFile }() // If there is at least one invalid pattern, which should not be silently tolerated. - if err := Start(context.Background()); err == nil { + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + if err := Start(ctx); err == nil { t.Fatalf("Invalid patterns not triggering error") } } @@ -316,7 +320,9 @@ func (s) TestBothConfigEnvVarsSet(t *testing.T) { defer func() { envconfig.ObservabilityConfig = oldObservabilityConfig }() - if err := Start(context.Background()); err == nil { + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + if err := Start(ctx); err == nil { t.Fatalf("Invalid patterns not triggering error") } } @@ -331,7 +337,9 @@ func (s) TestErrInFileSystemEnvVar(t *testing.T) { defer func() { envconfig.ObservabilityConfigFile = oldObservabilityConfigFile }() - if err := Start(context.Background()); err == nil { + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + if err := Start(ctx); err == nil { t.Fatalf("Invalid file system path not triggering error") } } @@ -346,7 +354,9 @@ func (s) TestNoEnvSet(t *testing.T) { envconfig.ObservabilityConfigFile = oldObservabilityConfigFile }() // If there is no observability config set at all, the Start should return an error. - if err := Start(context.Background()); err == nil { + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + if err := Start(ctx); err == nil { t.Fatalf("Invalid patterns not triggering error") } } @@ -540,7 +550,9 @@ func (s) TestStartErrorsThenEnd(t *testing.T) { envconfig.ObservabilityConfig = oldObservabilityConfig envconfig.ObservabilityConfigFile = oldObservabilityConfigFile }() - if err := Start(context.Background()); err == nil { + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + if err := Start(ctx); err == nil { t.Fatalf("Invalid patterns not triggering error") } End() From a1e7035d8cea3ceaed5715b501e1aafeff0e59b5 Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Wed, 19 Jun 2024 17:53:36 +0300 Subject: [PATCH 07/37] ignore credentials/google package as this package doesnt have default timeout defined --- scripts/vet.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/vet.sh b/scripts/vet.sh index 43081b0bac6f..e53401277596 100755 --- a/scripts/vet.sh +++ b/scripts/vet.sh @@ -51,7 +51,7 @@ git grep 'func (s) ' -- "*_test.go" | not grep -v 'func (s) Test' git grep 'func [A-Z]' -- "*_test.go" | not grep -v 'func Test\|Benchmark\|Example' # - Make sure all context usages are done timeout. -git grep -e 'context.Background()' --or -e 'context.TODO()' -- "*_test.go" | grep -v "benchmark/primitives/context_test.go" | not grep -v 'context.WithTimeout(context.Background()' | not grep -v 'context.WithTimeOut(context.TODO()' +git grep -e 'context.Background()' --or -e 'context.TODO()' -- "*_test.go" | grep -v "benchmark/primitives/context_test.go" | grep -v "credentials/google" | not grep -v 'context.WithTimeout(context.Background()' | not grep -v 'context.WithTimeOut(context.TODO()' # - Do not use time.After except in tests. It has the potential to leak the # timer since there is no way to stop it early. From 4a7ec24fdd85dd41015501940c4ee2cc5d113811 Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Wed, 19 Jun 2024 18:03:56 +0300 Subject: [PATCH 08/37] Change all context in bundle_ext_test.go to context with timeout --- internal/xds/bootstrap/tlscreds/bundle_ext_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/xds/bootstrap/tlscreds/bundle_ext_test.go b/internal/xds/bootstrap/tlscreds/bundle_ext_test.go index c6cd40246219..6747f75257b6 100644 --- a/internal/xds/bootstrap/tlscreds/bundle_ext_test.go +++ b/internal/xds/bootstrap/tlscreds/bundle_ext_test.go @@ -247,7 +247,9 @@ func (s) TestMTLS(t *testing.T) { } defer conn.Close() client := testgrpc.NewTestServiceClient(conn) - if _, err = client.EmptyCall(context.Background(), &testpb.Empty{}); err != nil { + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + if _, err = client.EmptyCall(ctx, &testpb.Empty{}); err != nil { t.Errorf("EmptyCall(): got error %v when expected to succeed", err) } } From 912de44b4f8f55f81881ee8abdb9692d1c2b59f0 Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Wed, 19 Jun 2024 18:06:53 +0300 Subject: [PATCH 09/37] Change all context in bundle_test.go to context with timeout --- internal/xds/bootstrap/tlscreds/bundle_test.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/internal/xds/bootstrap/tlscreds/bundle_test.go b/internal/xds/bootstrap/tlscreds/bundle_test.go index b4bee7f2f516..db713f540a80 100644 --- a/internal/xds/bootstrap/tlscreds/bundle_test.go +++ b/internal/xds/bootstrap/tlscreds/bundle_test.go @@ -25,6 +25,7 @@ import ( "fmt" "strings" "testing" + "time" "google.golang.org/grpc" "google.golang.org/grpc/credentials/tls/certprovider" @@ -36,6 +37,8 @@ import ( "google.golang.org/grpc/testdata" ) +const defaultTestTimeout = 5 * time.Second + type s struct { grpctest.Tester } @@ -85,7 +88,9 @@ func (s) TestFailingProvider(t *testing.T) { defer conn.Close() client := testgrpc.NewTestServiceClient(conn) - _, err = client.EmptyCall(context.Background(), &testpb.Empty{}) + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + _, err = client.EmptyCall(ctx, &testpb.Empty{}) if wantErr := "test error"; err == nil || !strings.Contains(err.Error(), wantErr) { t.Errorf("EmptyCall() got err: %s, want err to contain: %s", err, wantErr) } From 150515ffde553ef4b8e1d5ca32d54ccfafd86fbd Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Wed, 19 Jun 2024 18:11:46 +0300 Subject: [PATCH 10/37] Change rbac_engine_test context to context with timeout --- internal/xds/rbac/rbac_engine_test.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/internal/xds/rbac/rbac_engine_test.go b/internal/xds/rbac/rbac_engine_test.go index 94f12e92400a..0ea428bd0503 100644 --- a/internal/xds/rbac/rbac_engine_test.go +++ b/internal/xds/rbac/rbac_engine_test.go @@ -27,6 +27,7 @@ import ( "net/url" "reflect" "testing" + "time" v1xdsudpatypepb "github.com/cncf/xds/go/udpa/type/v1" v3xdsxdstypepb "github.com/cncf/xds/go/xds/type/v3" @@ -48,6 +49,8 @@ import ( "google.golang.org/protobuf/types/known/wrapperspb" ) +const defaultTestTimeout = 10 * time.Second + type s struct { grpctest.Tester } @@ -1748,8 +1751,8 @@ func (s) TestChainEngine(t *testing.T) { // information to represent incoming RPC's. This will be how a // user uses this API. A user will have to put MD, PeerInfo, and // the connection the RPC is sent on in the context. - ctx := metadata.NewIncomingContext(context.Background(), data.rpcData.md) - + ctx, cancel := context.WithTimeout(metadata.NewIncomingContext(context.Background(), data.rpcData.md), defaultTestTimeout) + defer cancel() // Make a TCP connection with a certain destination port. The // address/port of this connection will be used to populate the // destination ip/port in RPCData struct. This represents what From 9cb967d0a10eb4d626d803974a1cc2e4435867ed Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Thu, 20 Jun 2024 22:42:40 +0300 Subject: [PATCH 11/37] add context with timeout to method_logger_test in binary log --- internal/binarylog/method_logger_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/internal/binarylog/method_logger_test.go b/internal/binarylog/method_logger_test.go index d892a45669d7..767f744e11c5 100644 --- a/internal/binarylog/method_logger_test.go +++ b/internal/binarylog/method_logger_test.go @@ -33,6 +33,8 @@ import ( "google.golang.org/protobuf/types/known/durationpb" ) +const defaultTestTimeout = 10 * time.Second + func (s) TestLog(t *testing.T) { idGen.reset() ml := NewTruncatingMethodLogger(10, 10) @@ -336,7 +338,9 @@ func (s) TestLog(t *testing.T) { for i, tc := range testCases { buf.Reset() tc.want.SequenceIdWithinCall = uint64(i + 1) - ml.Log(context.Background(), tc.config) + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + ml.Log(ctx, tc.config) inSink := new(binlogpb.GrpcLogEntry) if err := proto.Unmarshal(buf.Bytes()[4:], inSink); err != nil { t.Errorf("failed to unmarshal bytes in sink to proto: %v", err) From 53cc2706f10d807bf1eb153293aaf8c0299c42cc Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Thu, 20 Jun 2024 22:45:28 +0300 Subject: [PATCH 12/37] adding internal/transport package to avoid list in linter rule --- scripts/vet.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/vet.sh b/scripts/vet.sh index e53401277596..e856aba60d40 100755 --- a/scripts/vet.sh +++ b/scripts/vet.sh @@ -51,7 +51,7 @@ git grep 'func (s) ' -- "*_test.go" | not grep -v 'func (s) Test' git grep 'func [A-Z]' -- "*_test.go" | not grep -v 'func Test\|Benchmark\|Example' # - Make sure all context usages are done timeout. -git grep -e 'context.Background()' --or -e 'context.TODO()' -- "*_test.go" | grep -v "benchmark/primitives/context_test.go" | grep -v "credentials/google" | not grep -v 'context.WithTimeout(context.Background()' | not grep -v 'context.WithTimeOut(context.TODO()' +git grep -e 'context.Background()' --or -e 'context.TODO()' -- "*_test.go" | grep -v "benchmark/primitives/context_test.go" | grep -v "credentials/google" | grep -v "internal/transport/" | not grep -v 'context.WithTimeout(context.Background()' | not grep -v 'context.WithTimeOut(context.TODO()' # - Do not use time.After except in tests. It has the potential to leak the # timer since there is no way to stop it early. From befaa3d58008fa2272bd3725ad71ff829a2e6dc5 Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Thu, 20 Jun 2024 22:49:01 +0300 Subject: [PATCH 13/37] reduce rule in linter to avoid catching cases were context.WithTimeout is coming before metadata --- scripts/vet.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/vet.sh b/scripts/vet.sh index e856aba60d40..9a3a9e8064b5 100755 --- a/scripts/vet.sh +++ b/scripts/vet.sh @@ -51,7 +51,7 @@ git grep 'func (s) ' -- "*_test.go" | not grep -v 'func (s) Test' git grep 'func [A-Z]' -- "*_test.go" | not grep -v 'func Test\|Benchmark\|Example' # - Make sure all context usages are done timeout. -git grep -e 'context.Background()' --or -e 'context.TODO()' -- "*_test.go" | grep -v "benchmark/primitives/context_test.go" | grep -v "credentials/google" | grep -v "internal/transport/" | not grep -v 'context.WithTimeout(context.Background()' | not grep -v 'context.WithTimeOut(context.TODO()' +git grep -e 'context.Background()' --or -e 'context.TODO()' -- "*_test.go" | grep -v "benchmark/primitives/context_test.go" | grep -v "credentials/google" | grep -v "internal/transport/" | not grep -v 'context.WithTimeout(' | not grep -v 'context.WithCancel(' # - Do not use time.After except in tests. It has the potential to leak the # timer since there is no way to stop it early. From eaccddae8d111affff0208704c484a7872c61142 Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Thu, 20 Jun 2024 22:53:55 +0300 Subject: [PATCH 14/37] change all context usage in metadata_test to context.WithTimeout --- metadata/metadata_test.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/metadata/metadata_test.go b/metadata/metadata_test.go index 6753764b9ba3..58488f6a3e2a 100644 --- a/metadata/metadata_test.go +++ b/metadata/metadata_test.go @@ -205,7 +205,8 @@ func (s) TestFromIncomingContext(t *testing.T) { ) // Verify that we lowercase if callers directly modify md md["X-INCORRECT-UPPERCASE"] = []string{"foo"} - ctx := NewIncomingContext(context.Background(), md) + ctx, cancel := context.WithTimeout(NewIncomingContext(context.Background(), md), defaultTestTimeout) + defer cancel() result, found := FromIncomingContext(ctx) if !found { @@ -241,7 +242,8 @@ func (s) TestValueFromIncomingContext(t *testing.T) { ) // Verify that we lowercase if callers directly modify md md["X-INCORRECT-UPPERCASE"] = []string{"foo"} - ctx := NewIncomingContext(context.Background(), md) + ctx, cancel := context.WithTimeout(NewIncomingContext(context.Background(), md), defaultTestTimeout) + defer cancel() for _, test := range []struct { key string @@ -398,7 +400,9 @@ func BenchmarkFromOutgoingContext(b *testing.B) { func BenchmarkFromIncomingContext(b *testing.B) { md := Pairs("X-My-Header-1", "42") - ctx := NewIncomingContext(context.Background(), md) + ctx, cancel := context.WithTimeout(NewIncomingContext(context.Background(), md), defaultTestTimeout) + defer cancel() + b.ResetTimer() for n := 0; n < b.N; n++ { FromIncomingContext(ctx) @@ -407,7 +411,8 @@ func BenchmarkFromIncomingContext(b *testing.B) { func BenchmarkValueFromIncomingContext(b *testing.B) { md := Pairs("X-My-Header-1", "42") - ctx := NewIncomingContext(context.Background(), md) + ctx, cancel := context.WithTimeout(NewIncomingContext(context.Background(), md), defaultTestTimeout) + defer cancel() b.Run("key-found", func(b *testing.B) { for n := 0; n < b.N; n++ { From a4e774252dd10f173601eee47aac576b700bbc69 Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Thu, 20 Jun 2024 22:55:56 +0300 Subject: [PATCH 15/37] add xds/internal to avoid list of linter rule --- scripts/vet.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/vet.sh b/scripts/vet.sh index 9a3a9e8064b5..5f7afcf80622 100755 --- a/scripts/vet.sh +++ b/scripts/vet.sh @@ -51,7 +51,7 @@ git grep 'func (s) ' -- "*_test.go" | not grep -v 'func (s) Test' git grep 'func [A-Z]' -- "*_test.go" | not grep -v 'func Test\|Benchmark\|Example' # - Make sure all context usages are done timeout. -git grep -e 'context.Background()' --or -e 'context.TODO()' -- "*_test.go" | grep -v "benchmark/primitives/context_test.go" | grep -v "credentials/google" | grep -v "internal/transport/" | not grep -v 'context.WithTimeout(' | not grep -v 'context.WithCancel(' +git grep -e 'context.Background()' --or -e 'context.TODO()' -- "*_test.go" | grep -v "benchmark/primitives/context_test.go" | grep -v "credentials/google" | grep -v "internal/transport/" | grep -v "xds/internal/" | not grep -v 'context.WithTimeout(' | not grep -v 'context.WithCancel(' # - Do not use time.After except in tests. It has the potential to leak the # timer since there is no way to stop it early. From aa71ff0f85474da58954f4a3517657c7fe148caa Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Thu, 20 Jun 2024 22:58:18 +0300 Subject: [PATCH 16/37] add context.WithTimeout to peer_test --- peer/peer_test.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/peer/peer_test.go b/peer/peer_test.go index c763ff4a29b6..81814fcc0a13 100644 --- a/peer/peer_test.go +++ b/peer/peer_test.go @@ -22,10 +22,13 @@ import ( "context" "fmt" "testing" + "time" "google.golang.org/grpc/credentials" ) +const defaultTestTimeout = 10 * time.Second + // A struct that implements AuthInfo interface and implements CommonAuthInfo() method. type testAuthInfo struct { credentials.CommonAuthInfo @@ -82,7 +85,9 @@ func TestPeerStringer(t *testing.T) { } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - ctx := NewContext(context.Background(), tc.peer) + ctx, cancel := context.WithTimeout(NewContext(context.Background(), tc.peer), defaultTestTimeout) + defer cancel() + p, ok := FromContext(ctx) if !ok { t.Fatalf("Unable to get peer from context") From d1aefe7c839ba2fb4f798ac603fc654f01e14a6d Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Thu, 20 Jun 2024 23:04:19 +0300 Subject: [PATCH 17/37] change all context usage in picker_wrapper_test to context.WithTimeout --- picker_wrapper_test.go | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/picker_wrapper_test.go b/picker_wrapper_test.go index ba99a06b0f8c..12aaec56ff78 100644 --- a/picker_wrapper_test.go +++ b/picker_wrapper_test.go @@ -32,7 +32,9 @@ import ( "google.golang.org/grpc/status" ) -const goroutineCount = 5 +const ( + goroutineCount = 5 +) var ( testT = &testTransport{} @@ -80,7 +82,10 @@ func (s) TestBlockingPick(t *testing.T) { var finishedCount uint64 for i := goroutineCount; i > 0; i-- { go func() { - if tr, _, err := bp.pick(context.Background(), true, balancer.PickInfo{}); err != nil || tr != testT { + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + + if tr, _, err := bp.pick(ctx, true, balancer.PickInfo{}); err != nil || tr != testT { t.Errorf("bp.pick returned non-nil error: %v", err) } atomic.AddUint64(&finishedCount, 1) @@ -100,7 +105,10 @@ func (s) TestBlockingPickNoSubAvailable(t *testing.T) { // All goroutines should block because picker returns no subConn available. for i := goroutineCount; i > 0; i-- { go func() { - if tr, _, err := bp.pick(context.Background(), true, balancer.PickInfo{}); err != nil || tr != testT { + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + + if tr, _, err := bp.pick(ctx, true, balancer.PickInfo{}); err != nil || tr != testT { t.Errorf("bp.pick returned non-nil error: %v", err) } atomic.AddUint64(&finishedCount, 1) @@ -121,7 +129,10 @@ func (s) TestBlockingPickTransientWaitforready(t *testing.T) { // picks are not failfast. for i := goroutineCount; i > 0; i-- { go func() { - if tr, _, err := bp.pick(context.Background(), false, balancer.PickInfo{}); err != nil || tr != testT { + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + + if tr, _, err := bp.pick(ctx, false, balancer.PickInfo{}); err != nil || tr != testT { t.Errorf("bp.pick returned non-nil error: %v", err) } atomic.AddUint64(&finishedCount, 1) @@ -141,7 +152,10 @@ func (s) TestBlockingPickSCNotReady(t *testing.T) { // All goroutines should block because subConn is not ready. for i := goroutineCount; i > 0; i-- { go func() { - if tr, _, err := bp.pick(context.Background(), true, balancer.PickInfo{}); err != nil || tr != testT { + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + + if tr, _, err := bp.pick(ctx, true, balancer.PickInfo{}); err != nil || tr != testT { t.Errorf("bp.pick returned non-nil error: %v", err) } atomic.AddUint64(&finishedCount, 1) From 40a54fb900af8d61823ce2ac2403ae563166759c Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Thu, 20 Jun 2024 23:06:30 +0300 Subject: [PATCH 18/37] add security/advancedtls to avoid list in linter rule --- scripts/vet.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/vet.sh b/scripts/vet.sh index 5f7afcf80622..d0b623edb0ea 100755 --- a/scripts/vet.sh +++ b/scripts/vet.sh @@ -51,7 +51,7 @@ git grep 'func (s) ' -- "*_test.go" | not grep -v 'func (s) Test' git grep 'func [A-Z]' -- "*_test.go" | not grep -v 'func Test\|Benchmark\|Example' # - Make sure all context usages are done timeout. -git grep -e 'context.Background()' --or -e 'context.TODO()' -- "*_test.go" | grep -v "benchmark/primitives/context_test.go" | grep -v "credentials/google" | grep -v "internal/transport/" | grep -v "xds/internal/" | not grep -v 'context.WithTimeout(' | not grep -v 'context.WithCancel(' +git grep -e 'context.Background()' --or -e 'context.TODO()' -- "*_test.go" | grep -v "benchmark/primitives/context_test.go" | grep -v "credentials/google" | grep -v "internal/transport/" | grep -v "xds/internal/" | grep -v "security/advancedtls" | not grep -v 'context.WithTimeout(' | not grep -v 'context.WithCancel(' # - Do not use time.After except in tests. It has the potential to leak the # timer since there is no way to stop it early. From 96345b1e52b62fdb10a1e4267d34f37ed02d22f5 Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Thu, 20 Jun 2024 23:14:38 +0300 Subject: [PATCH 19/37] change all usage of context in server_test.go to context.WithTimeout --- server_test.go | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/server_test.go b/server_test.go index 0048a0f74156..c73ed650b64a 100644 --- a/server_test.go +++ b/server_test.go @@ -153,7 +153,11 @@ func (s) TestRetryChainedInterceptor(t *testing.T) { handler := func(ctx context.Context, req any) (any, error) { return nil, nil } - ii(context.Background(), nil, nil, handler) + + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + + ii(ctx, nil, nil, handler) if !cmp.Equal(records, []int{1, 2, 3, 2, 3}) { t.Fatalf("retry failed on chained interceptors: %v", records) } @@ -161,7 +165,9 @@ func (s) TestRetryChainedInterceptor(t *testing.T) { func (s) TestStreamContext(t *testing.T) { expectedStream := &transport.Stream{} - ctx := NewContextWithServerTransportStream(context.Background(), expectedStream) + ctx, cancel := context.WithTimeout(NewContextWithServerTransportStream(context.Background(), expectedStream), defaultTestTimeout) + defer cancel() + s := ServerTransportStreamFromContext(ctx) stream, ok := s.(*transport.Stream) if !ok || expectedStream != stream { @@ -186,7 +192,10 @@ func BenchmarkChainUnaryInterceptor(b *testing.B) { b.ReportAllocs() b.ResetTimer() for i := 0; i < b.N; i++ { - if _, err := s.opts.unaryInt(context.Background(), nil, nil, + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + + if _, err := s.opts.unaryInt(ctx, nil, nil, func(ctx context.Context, req any) (any, error) { return nil, nil }, From 59a3855795fb436863d2ea3375c772e9e2602465 Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Thu, 20 Jun 2024 23:16:42 +0300 Subject: [PATCH 20/37] change missing context usage in observability_test to context.WithTimeout --- stats/opentelemetry/csm/observability_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/stats/opentelemetry/csm/observability_test.go b/stats/opentelemetry/csm/observability_test.go index efa97199f530..b53e23d4aade 100644 --- a/stats/opentelemetry/csm/observability_test.go +++ b/stats/opentelemetry/csm/observability_test.go @@ -606,6 +606,9 @@ func (s) TestXDSLabels(t *testing.T) { // without error. The actual functionality of this function will be verified in // interop tests. func (s) TestObservability(t *testing.T) { - cleanup := EnableObservability(context.Background(), opentelemetry.Options{}) + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + + cleanup := EnableObservability(ctx, opentelemetry.Options{}) cleanup() } From cfc815b8122444250edc6cb5da4a6f9823b3f654 Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Thu, 20 Jun 2024 23:20:28 +0300 Subject: [PATCH 21/37] change all context usage to context.WithTimeout in gracefulstop_test --- test/gracefulstop_test.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/gracefulstop_test.go b/test/gracefulstop_test.go index ecf07d984359..0e5c760648af 100644 --- a/test/gracefulstop_test.go +++ b/test/gracefulstop_test.go @@ -241,7 +241,9 @@ func (s) TestGracefulStopBlocksUntilGRPCConnectionsTerminate(t *testing.T) { grpcClientCallReturned := make(chan struct{}) go func() { clt := ss.Client - _, err := clt.UnaryCall(context.Background(), &testpb.SimpleRequest{}) + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + _, err := clt.UnaryCall(ctx, &testpb.SimpleRequest{}) if err != nil { t.Errorf("rpc failed with error: %s", err) } @@ -290,7 +292,9 @@ func (s) TestStopAbortsBlockingGRPCCall(t *testing.T) { grpcClientCallReturned := make(chan struct{}) go func() { clt := ss.Client - _, err := clt.UnaryCall(context.Background(), &testpb.SimpleRequest{}) + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + _, err := clt.UnaryCall(ctx, &testpb.SimpleRequest{}) if err == nil || !isConnClosedErr(err) { t.Errorf("expected rpc to fail with connection closed error, got: %v", err) } From c33cbef54abe0e688c63b4249706291df7859520 Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Thu, 20 Jun 2024 23:36:39 +0300 Subject: [PATCH 22/37] remove added lint from vet.sh --- scripts/vet.sh | 3 --- 1 file changed, 3 deletions(-) diff --git a/scripts/vet.sh b/scripts/vet.sh index d0b623edb0ea..f88529e7d146 100755 --- a/scripts/vet.sh +++ b/scripts/vet.sh @@ -50,9 +50,6 @@ not grep 'func Test[^(]' -- test/*.go git grep 'func (s) ' -- "*_test.go" | not grep -v 'func (s) Test' git grep 'func [A-Z]' -- "*_test.go" | not grep -v 'func Test\|Benchmark\|Example' -# - Make sure all context usages are done timeout. -git grep -e 'context.Background()' --or -e 'context.TODO()' -- "*_test.go" | grep -v "benchmark/primitives/context_test.go" | grep -v "credentials/google" | grep -v "internal/transport/" | grep -v "xds/internal/" | grep -v "security/advancedtls" | not grep -v 'context.WithTimeout(' | not grep -v 'context.WithCancel(' - # - Do not use time.After except in tests. It has the potential to leak the # timer since there is no way to stop it early. git grep -l 'time.After(' -- "*.go" | not grep -v '_test.go\|test_utils\|testutils' From e7192b1841c9d1e1c5399726100c985d774b0a42 Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Thu, 20 Jun 2024 23:42:52 +0300 Subject: [PATCH 23/37] add lint rule of all context should have timeouts back --- scripts/vet.sh | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/scripts/vet.sh b/scripts/vet.sh index f88529e7d146..6a13fdc3a25c 100755 --- a/scripts/vet.sh +++ b/scripts/vet.sh @@ -69,6 +69,10 @@ not git grep "\(import \|^\s*\)\"google.golang.org/grpc/interop/grpc_testing" -- # - Ensure all xds proto imports are renamed to *pb or *grpc. git grep '"github.com/envoyproxy/go-control-plane/envoy' -- '*.go' ':(exclude)*.pb.go' | not grep -v 'pb "\|grpc "' +# - Ensure all context usages are done with timeout. +git grep -e 'context.Background()' --or -e 'context.TODO()' -- "*_test.go" | grep -v "benchmark/primitives/context_test.go" | grep -v "credentials/google" | grep -v "internal/transport/" | grep -v "xds/internal/" | grep -v "security/advancedtls" | not grep -v 'context.WithTimeout(' | not grep -v 'context.WithCancel(' + + misspell -error . # - gofmt, goimports, go vet, go mod tidy. From 689ae5b29b15c3012906d20e42718a69a7a31e52 Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Thu, 20 Jun 2024 23:49:21 +0300 Subject: [PATCH 24/37] change lint rule in all avoided packages to not grep --- scripts/vet.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/vet.sh b/scripts/vet.sh index 6a13fdc3a25c..60d98fcf2351 100755 --- a/scripts/vet.sh +++ b/scripts/vet.sh @@ -70,7 +70,7 @@ not git grep "\(import \|^\s*\)\"google.golang.org/grpc/interop/grpc_testing" -- git grep '"github.com/envoyproxy/go-control-plane/envoy' -- '*.go' ':(exclude)*.pb.go' | not grep -v 'pb "\|grpc "' # - Ensure all context usages are done with timeout. -git grep -e 'context.Background()' --or -e 'context.TODO()' -- "*_test.go" | grep -v "benchmark/primitives/context_test.go" | grep -v "credentials/google" | grep -v "internal/transport/" | grep -v "xds/internal/" | grep -v "security/advancedtls" | not grep -v 'context.WithTimeout(' | not grep -v 'context.WithCancel(' +git grep -e 'context.Background()' --or -e 'context.TODO()' -- "*_test.go" | not grep -v "benchmark/primitives/context_test.go" | not grep -v "credentials/google" | not grep -v "internal/transport/" | not grep -v "xds/internal/" | not grep -v "security/advancedtls" | not grep -v 'context.WithTimeout(' | not grep -v 'context.WithCancel(' misspell -error . From 6a51d0d6584e649825ef0569c70a4d8936f212b0 Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Thu, 20 Jun 2024 23:55:54 +0300 Subject: [PATCH 25/37] change lint rule to not to enforce that the grep returns nothing --- scripts/vet.sh | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/scripts/vet.sh b/scripts/vet.sh index 60d98fcf2351..945e6dc9f927 100755 --- a/scripts/vet.sh +++ b/scripts/vet.sh @@ -70,8 +70,9 @@ not git grep "\(import \|^\s*\)\"google.golang.org/grpc/interop/grpc_testing" -- git grep '"github.com/envoyproxy/go-control-plane/envoy' -- '*.go' ':(exclude)*.pb.go' | not grep -v 'pb "\|grpc "' # - Ensure all context usages are done with timeout. -git grep -e 'context.Background()' --or -e 'context.TODO()' -- "*_test.go" | not grep -v "benchmark/primitives/context_test.go" | not grep -v "credentials/google" | not grep -v "internal/transport/" | not grep -v "xds/internal/" | not grep -v "security/advancedtls" | not grep -v 'context.WithTimeout(' | not grep -v 'context.WithCancel(' - +not git grep -e 'context.Background()' --or -e 'context.TODO()' -- "*_test.go" | grep -v "benchmark/primitives/context_test.go" | grep -v "credential +s/google" | grep -v "internal/transport/" | grep -v "xds/internal/" | grep -v "security/advancedtls" | grep -v 'context.WithTimeout(' | grep -v 'contex +t.WithCancel(' misspell -error . From c29c45c357c8678f792107033d851155d19c2668 Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Thu, 20 Jun 2024 23:55:54 +0300 Subject: [PATCH 26/37] change lint rule to not to enforce that the grep returns nothing --- scripts/vet.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/vet.sh b/scripts/vet.sh index 60d98fcf2351..fc53a79e96e1 100755 --- a/scripts/vet.sh +++ b/scripts/vet.sh @@ -70,8 +70,8 @@ not git grep "\(import \|^\s*\)\"google.golang.org/grpc/interop/grpc_testing" -- git grep '"github.com/envoyproxy/go-control-plane/envoy' -- '*.go' ':(exclude)*.pb.go' | not grep -v 'pb "\|grpc "' # - Ensure all context usages are done with timeout. -git grep -e 'context.Background()' --or -e 'context.TODO()' -- "*_test.go" | not grep -v "benchmark/primitives/context_test.go" | not grep -v "credentials/google" | not grep -v "internal/transport/" | not grep -v "xds/internal/" | not grep -v "security/advancedtls" | not grep -v 'context.WithTimeout(' | not grep -v 'context.WithCancel(' - +git grep -e 'context.Background()' --or -e 'context.TODO()' -- "*_test.go" | grep -v "benchmark/primitives/context_test.go" | grep -v "credential +s/google" | grep -v "internal/transport/" | grep -v "xds/internal/" | grep -v "security/advancedtls" | grep -v 'context.WithTimeout(' | not grep -v 'context.WithCancel(' misspell -error . From f0f09520978f36a28da66b31aa558ac55d0c2194 Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Thu, 27 Jun 2024 23:56:27 +0300 Subject: [PATCH 27/37] add clarifications to the comment above the lint rule --- scripts/vet.sh | 3 +++ 1 file changed, 3 insertions(+) diff --git a/scripts/vet.sh b/scripts/vet.sh index fc53a79e96e1..18368261f7b5 100755 --- a/scripts/vet.sh +++ b/scripts/vet.sh @@ -70,6 +70,9 @@ not git grep "\(import \|^\s*\)\"google.golang.org/grpc/interop/grpc_testing" -- git grep '"github.com/envoyproxy/go-control-plane/envoy' -- '*.go' ':(exclude)*.pb.go' | not grep -v 'pb "\|grpc "' # - Ensure all context usages are done with timeout. +# Context tests under benchmark are excluded as they are testing the performance of context.Background() and context.TODO(). +# TODO: Remove the exclusions once the tests are updated to use context.WithTimeout(). +# See https://github.com/grpc/grpc-go/issues/7304 git grep -e 'context.Background()' --or -e 'context.TODO()' -- "*_test.go" | grep -v "benchmark/primitives/context_test.go" | grep -v "credential s/google" | grep -v "internal/transport/" | grep -v "xds/internal/" | grep -v "security/advancedtls" | grep -v 'context.WithTimeout(' | not grep -v 'context.WithCancel(' From 0430481ff91d00c0c0383439d23b7d0b26fe96d8 Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Fri, 28 Jun 2024 00:00:55 +0300 Subject: [PATCH 28/37] revert const change in test --- picker_wrapper_test.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/picker_wrapper_test.go b/picker_wrapper_test.go index 12aaec56ff78..513d859e172b 100644 --- a/picker_wrapper_test.go +++ b/picker_wrapper_test.go @@ -32,9 +32,7 @@ import ( "google.golang.org/grpc/status" ) -const ( - goroutineCount = 5 -) +const goroutineCount = 5 var ( testT = &testTransport{} From 832789be038ffd3bbd1d9ba6e2d8a7e30deeb34b Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Fri, 28 Jun 2024 00:09:55 +0300 Subject: [PATCH 29/37] move context creation to improve test readability --- internal/xds/rbac/rbac_engine_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/xds/rbac/rbac_engine_test.go b/internal/xds/rbac/rbac_engine_test.go index 0ea428bd0503..a7eefedfc307 100644 --- a/internal/xds/rbac/rbac_engine_test.go +++ b/internal/xds/rbac/rbac_engine_test.go @@ -1747,12 +1747,13 @@ func (s) TestChainEngine(t *testing.T) { // if the chain of RBAC Engines configured as such works as intended. for _, data := range test.rbacQueries { func() { + ctxWithTimeout, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() // Construct the context with three data points that have enough // information to represent incoming RPC's. This will be how a // user uses this API. A user will have to put MD, PeerInfo, and // the connection the RPC is sent on in the context. - ctx, cancel := context.WithTimeout(metadata.NewIncomingContext(context.Background(), data.rpcData.md), defaultTestTimeout) - defer cancel() + ctx := metadata.NewIncomingContext(ctxWithTimeout, data.rpcData.md) // Make a TCP connection with a certain destination port. The // address/port of this connection will be used to populate the // destination ip/port in RPCData struct. This represents what From 9719207a22e075af0fa75a3791e9edb9e8e51ad4 Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Fri, 28 Jun 2024 15:38:14 +0300 Subject: [PATCH 30/37] extracted context with timeout setup to central location to improve test readability --- metadata/metadata_test.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/metadata/metadata_test.go b/metadata/metadata_test.go index 58488f6a3e2a..b8dbde55ceac 100644 --- a/metadata/metadata_test.go +++ b/metadata/metadata_test.go @@ -31,6 +31,8 @@ import ( const defaultTestTimeout = 10 * time.Second +var ctxWithTimeout, cancel = context.WithTimeout(context.Background(), defaultTestTimeout) + type s struct { grpctest.Tester } @@ -205,7 +207,7 @@ func (s) TestFromIncomingContext(t *testing.T) { ) // Verify that we lowercase if callers directly modify md md["X-INCORRECT-UPPERCASE"] = []string{"foo"} - ctx, cancel := context.WithTimeout(NewIncomingContext(context.Background(), md), defaultTestTimeout) + ctx := NewIncomingContext(ctxWithTimeout, md) defer cancel() result, found := FromIncomingContext(ctx) @@ -242,7 +244,7 @@ func (s) TestValueFromIncomingContext(t *testing.T) { ) // Verify that we lowercase if callers directly modify md md["X-INCORRECT-UPPERCASE"] = []string{"foo"} - ctx, cancel := context.WithTimeout(NewIncomingContext(context.Background(), md), defaultTestTimeout) + ctx := NewIncomingContext(ctxWithTimeout, md) defer cancel() for _, test := range []struct { @@ -400,7 +402,7 @@ func BenchmarkFromOutgoingContext(b *testing.B) { func BenchmarkFromIncomingContext(b *testing.B) { md := Pairs("X-My-Header-1", "42") - ctx, cancel := context.WithTimeout(NewIncomingContext(context.Background(), md), defaultTestTimeout) + ctx := NewIncomingContext(ctxWithTimeout, md) defer cancel() b.ResetTimer() @@ -411,7 +413,7 @@ func BenchmarkFromIncomingContext(b *testing.B) { func BenchmarkValueFromIncomingContext(b *testing.B) { md := Pairs("X-My-Header-1", "42") - ctx, cancel := context.WithTimeout(NewIncomingContext(context.Background(), md), defaultTestTimeout) + ctx := NewIncomingContext(ctxWithTimeout, md) defer cancel() b.Run("key-found", func(b *testing.B) { From 3081de9529869a5ac91d14b49e50ba0905b34556 Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Fri, 28 Jun 2024 15:41:01 +0300 Subject: [PATCH 31/37] improve test readability --- peer/peer_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/peer/peer_test.go b/peer/peer_test.go index 81814fcc0a13..7d4ccbaa8663 100644 --- a/peer/peer_test.go +++ b/peer/peer_test.go @@ -85,8 +85,9 @@ func TestPeerStringer(t *testing.T) { } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - ctx, cancel := context.WithTimeout(NewContext(context.Background(), tc.peer), defaultTestTimeout) + ctxWithTimeout, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) defer cancel() + ctx := NewContext(ctxWithTimeout, tc.peer) p, ok := FromContext(ctx) if !ok { From cb43ba0ffc16e152c9029c8c019e6d0ea4a9664b Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Fri, 28 Jun 2024 16:07:02 +0300 Subject: [PATCH 32/37] improve stream context test readability --- server_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server_test.go b/server_test.go index c73ed650b64a..20b336c28291 100644 --- a/server_test.go +++ b/server_test.go @@ -165,8 +165,9 @@ func (s) TestRetryChainedInterceptor(t *testing.T) { func (s) TestStreamContext(t *testing.T) { expectedStream := &transport.Stream{} - ctx, cancel := context.WithTimeout(NewContextWithServerTransportStream(context.Background(), expectedStream), defaultTestTimeout) + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) defer cancel() + ctx = NewContextWithServerTransportStream(ctx, expectedStream) s := ServerTransportStreamFromContext(ctx) stream, ok := s.(*transport.Stream) From 6c250727fb2839e881a20bc77dc4ad5a091850a9 Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Fri, 28 Jun 2024 23:42:23 +0300 Subject: [PATCH 33/37] rename ctxWithTimeout to ctx --- internal/xds/rbac/rbac_engine_test.go | 4 ++-- metadata/metadata_test.go | 10 +++++----- peer/peer_test.go | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/internal/xds/rbac/rbac_engine_test.go b/internal/xds/rbac/rbac_engine_test.go index a7eefedfc307..fedf2ae71017 100644 --- a/internal/xds/rbac/rbac_engine_test.go +++ b/internal/xds/rbac/rbac_engine_test.go @@ -1747,13 +1747,13 @@ func (s) TestChainEngine(t *testing.T) { // if the chain of RBAC Engines configured as such works as intended. for _, data := range test.rbacQueries { func() { - ctxWithTimeout, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) defer cancel() // Construct the context with three data points that have enough // information to represent incoming RPC's. This will be how a // user uses this API. A user will have to put MD, PeerInfo, and // the connection the RPC is sent on in the context. - ctx := metadata.NewIncomingContext(ctxWithTimeout, data.rpcData.md) + ctx = metadata.NewIncomingContext(ctx, data.rpcData.md) // Make a TCP connection with a certain destination port. The // address/port of this connection will be used to populate the // destination ip/port in RPCData struct. This represents what diff --git a/metadata/metadata_test.go b/metadata/metadata_test.go index b8dbde55ceac..3dd82b41af03 100644 --- a/metadata/metadata_test.go +++ b/metadata/metadata_test.go @@ -31,7 +31,7 @@ import ( const defaultTestTimeout = 10 * time.Second -var ctxWithTimeout, cancel = context.WithTimeout(context.Background(), defaultTestTimeout) +var ctx, cancel = context.WithTimeout(context.Background(), defaultTestTimeout) type s struct { grpctest.Tester @@ -207,7 +207,7 @@ func (s) TestFromIncomingContext(t *testing.T) { ) // Verify that we lowercase if callers directly modify md md["X-INCORRECT-UPPERCASE"] = []string{"foo"} - ctx := NewIncomingContext(ctxWithTimeout, md) + ctx := NewIncomingContext(ctx, md) defer cancel() result, found := FromIncomingContext(ctx) @@ -244,7 +244,7 @@ func (s) TestValueFromIncomingContext(t *testing.T) { ) // Verify that we lowercase if callers directly modify md md["X-INCORRECT-UPPERCASE"] = []string{"foo"} - ctx := NewIncomingContext(ctxWithTimeout, md) + ctx := NewIncomingContext(ctx, md) defer cancel() for _, test := range []struct { @@ -402,7 +402,7 @@ func BenchmarkFromOutgoingContext(b *testing.B) { func BenchmarkFromIncomingContext(b *testing.B) { md := Pairs("X-My-Header-1", "42") - ctx := NewIncomingContext(ctxWithTimeout, md) + ctx := NewIncomingContext(ctx, md) defer cancel() b.ResetTimer() @@ -413,7 +413,7 @@ func BenchmarkFromIncomingContext(b *testing.B) { func BenchmarkValueFromIncomingContext(b *testing.B) { md := Pairs("X-My-Header-1", "42") - ctx := NewIncomingContext(ctxWithTimeout, md) + ctx := NewIncomingContext(ctx, md) defer cancel() b.Run("key-found", func(b *testing.B) { diff --git a/peer/peer_test.go b/peer/peer_test.go index 7d4ccbaa8663..0ad3b48b71c7 100644 --- a/peer/peer_test.go +++ b/peer/peer_test.go @@ -85,9 +85,9 @@ func TestPeerStringer(t *testing.T) { } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - ctxWithTimeout, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) defer cancel() - ctx := NewContext(ctxWithTimeout, tc.peer) + ctx = NewContext(ctx, tc.peer) p, ok := FromContext(ctx) if !ok { From 4b897355fb291515d11b308c01245405345a206d Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Fri, 28 Jun 2024 23:44:32 +0300 Subject: [PATCH 34/37] move ctx to inside each test --- metadata/metadata_test.go | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/metadata/metadata_test.go b/metadata/metadata_test.go index 3dd82b41af03..23fe964ad8d4 100644 --- a/metadata/metadata_test.go +++ b/metadata/metadata_test.go @@ -31,8 +31,6 @@ import ( const defaultTestTimeout = 10 * time.Second -var ctx, cancel = context.WithTimeout(context.Background(), defaultTestTimeout) - type s struct { grpctest.Tester } @@ -205,10 +203,11 @@ func (s) TestFromIncomingContext(t *testing.T) { md := Pairs( "X-My-Header-1", "42", ) + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() // Verify that we lowercase if callers directly modify md md["X-INCORRECT-UPPERCASE"] = []string{"foo"} - ctx := NewIncomingContext(ctx, md) - defer cancel() + ctx = NewIncomingContext(ctx, md) result, found := FromIncomingContext(ctx) if !found { @@ -242,10 +241,11 @@ func (s) TestValueFromIncomingContext(t *testing.T) { "X-My-Header-2", "43-2", "x-my-header-3", "44", ) + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() // Verify that we lowercase if callers directly modify md md["X-INCORRECT-UPPERCASE"] = []string{"foo"} - ctx := NewIncomingContext(ctx, md) - defer cancel() + ctx = NewIncomingContext(ctx, md) for _, test := range []struct { key string @@ -401,9 +401,10 @@ func BenchmarkFromOutgoingContext(b *testing.B) { } func BenchmarkFromIncomingContext(b *testing.B) { - md := Pairs("X-My-Header-1", "42") - ctx := NewIncomingContext(ctx, md) + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) defer cancel() + md := Pairs("X-My-Header-1", "42") + ctx = NewIncomingContext(ctx, md) b.ResetTimer() for n := 0; n < b.N; n++ { @@ -412,9 +413,10 @@ func BenchmarkFromIncomingContext(b *testing.B) { } func BenchmarkValueFromIncomingContext(b *testing.B) { - md := Pairs("X-My-Header-1", "42") - ctx := NewIncomingContext(ctx, md) + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) defer cancel() + md := Pairs("X-My-Header-1", "42") + ctx = NewIncomingContext(ctx, md) b.Run("key-found", func(b *testing.B) { for n := 0; n < b.N; n++ { From fdca263dbca90b6fd9da0ba6514f8b641f0770a7 Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Wed, 3 Jul 2024 20:25:26 +0300 Subject: [PATCH 35/37] move context creation to outside of the loops --- internal/binarylog/method_logger_test.go | 4 ++-- internal/xds/rbac/rbac_engine_test.go | 4 ++-- peer/peer_test.go | 4 ++-- picker_wrapper_test.go | 5 ++--- 4 files changed, 8 insertions(+), 9 deletions(-) diff --git a/internal/binarylog/method_logger_test.go b/internal/binarylog/method_logger_test.go index 767f744e11c5..9b317c60ee17 100644 --- a/internal/binarylog/method_logger_test.go +++ b/internal/binarylog/method_logger_test.go @@ -335,11 +335,11 @@ func (s) TestLog(t *testing.T) { }, }, } + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() for i, tc := range testCases { buf.Reset() tc.want.SequenceIdWithinCall = uint64(i + 1) - ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) - defer cancel() ml.Log(ctx, tc.config) inSink := new(binlogpb.GrpcLogEntry) if err := proto.Unmarshal(buf.Bytes()[4:], inSink); err != nil { diff --git a/internal/xds/rbac/rbac_engine_test.go b/internal/xds/rbac/rbac_engine_test.go index fedf2ae71017..44f0e495864e 100644 --- a/internal/xds/rbac/rbac_engine_test.go +++ b/internal/xds/rbac/rbac_engine_test.go @@ -1745,10 +1745,10 @@ func (s) TestChainEngine(t *testing.T) { } // Query the created chain of RBAC Engines with different args to see // if the chain of RBAC Engines configured as such works as intended. + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() for _, data := range test.rbacQueries { func() { - ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) - defer cancel() // Construct the context with three data points that have enough // information to represent incoming RPC's. This will be how a // user uses this API. A user will have to put MD, PeerInfo, and diff --git a/peer/peer_test.go b/peer/peer_test.go index 0ad3b48b71c7..2ffff9b2a803 100644 --- a/peer/peer_test.go +++ b/peer/peer_test.go @@ -83,10 +83,10 @@ func TestPeerStringer(t *testing.T) { want: "Peer", }, } + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) - defer cancel() ctx = NewContext(ctx, tc.peer) p, ok := FromContext(ctx) diff --git a/picker_wrapper_test.go b/picker_wrapper_test.go index 513d859e172b..e863a5ab2a0f 100644 --- a/picker_wrapper_test.go +++ b/picker_wrapper_test.go @@ -78,11 +78,10 @@ func (s) TestBlockingPick(t *testing.T) { bp := newPickerWrapper(nil) // All goroutines should block because picker is nil in bp. var finishedCount uint64 + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() for i := goroutineCount; i > 0; i-- { go func() { - ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) - defer cancel() - if tr, _, err := bp.pick(ctx, true, balancer.PickInfo{}); err != nil || tr != testT { t.Errorf("bp.pick returned non-nil error: %v", err) } From d76f29c022ad6ca0954e3dbff187298031943d77 Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Wed, 3 Jul 2024 21:18:41 +0300 Subject: [PATCH 36/37] extract context creation from some more loops --- picker_wrapper_test.go | 15 ++++++--------- server_test.go | 5 ++--- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/picker_wrapper_test.go b/picker_wrapper_test.go index e863a5ab2a0f..33eb86f94b6f 100644 --- a/picker_wrapper_test.go +++ b/picker_wrapper_test.go @@ -99,12 +99,11 @@ func (s) TestBlockingPickNoSubAvailable(t *testing.T) { bp := newPickerWrapper(nil) var finishedCount uint64 bp.updatePicker(&testingPicker{err: balancer.ErrNoSubConnAvailable, maxCalled: goroutineCount}) + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() // All goroutines should block because picker returns no subConn available. for i := goroutineCount; i > 0; i-- { go func() { - ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) - defer cancel() - if tr, _, err := bp.pick(ctx, true, balancer.PickInfo{}); err != nil || tr != testT { t.Errorf("bp.pick returned non-nil error: %v", err) } @@ -122,13 +121,12 @@ func (s) TestBlockingPickTransientWaitforready(t *testing.T) { bp := newPickerWrapper(nil) bp.updatePicker(&testingPicker{err: balancer.ErrTransientFailure, maxCalled: goroutineCount}) var finishedCount uint64 + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() // All goroutines should block because picker returns transientFailure and // picks are not failfast. for i := goroutineCount; i > 0; i-- { go func() { - ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) - defer cancel() - if tr, _, err := bp.pick(ctx, false, balancer.PickInfo{}); err != nil || tr != testT { t.Errorf("bp.pick returned non-nil error: %v", err) } @@ -146,12 +144,11 @@ func (s) TestBlockingPickSCNotReady(t *testing.T) { bp := newPickerWrapper(nil) bp.updatePicker(&testingPicker{sc: testSCNotReady, maxCalled: goroutineCount}) var finishedCount uint64 + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() // All goroutines should block because subConn is not ready. for i := goroutineCount; i > 0; i-- { go func() { - ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) - defer cancel() - if tr, _, err := bp.pick(ctx, true, balancer.PickInfo{}); err != nil || tr != testT { t.Errorf("bp.pick returned non-nil error: %v", err) } diff --git a/server_test.go b/server_test.go index 20b336c28291..93560ae84a65 100644 --- a/server_test.go +++ b/server_test.go @@ -192,10 +192,9 @@ func BenchmarkChainUnaryInterceptor(b *testing.B) { s := NewServer(ChainUnaryInterceptor(interceptors...)) b.ReportAllocs() b.ResetTimer() + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() for i := 0; i < b.N; i++ { - ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) - defer cancel() - if _, err := s.opts.unaryInt(ctx, nil, nil, func(ctx context.Context, req any) (any, error) { return nil, nil From 136b4ba5c68dc9b9350bf20a0fce4c487e25c188 Mon Sep 17 00:00:00 2001 From: "user.name: \"Omer Hasson" Date: Wed, 3 Jul 2024 21:32:23 +0300 Subject: [PATCH 37/37] extract outside of all loops in bench mark server_test --- server_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server_test.go b/server_test.go index 93560ae84a65..8201fc8cfb5d 100644 --- a/server_test.go +++ b/server_test.go @@ -177,6 +177,8 @@ func (s) TestStreamContext(t *testing.T) { } func BenchmarkChainUnaryInterceptor(b *testing.B) { + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() for _, n := range []int{1, 3, 5, 10} { n := n b.Run(strconv.Itoa(n), func(b *testing.B) { @@ -192,8 +194,6 @@ func BenchmarkChainUnaryInterceptor(b *testing.B) { s := NewServer(ChainUnaryInterceptor(interceptors...)) b.ReportAllocs() b.ResetTimer() - ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) - defer cancel() for i := 0; i < b.N; i++ { if _, err := s.opts.unaryInt(ctx, nil, nil, func(ctx context.Context, req any) (any, error) {