interop: Replace context.Background() with passed ctx #6827

Aditya-Sood · 2023-12-01T06:14:19Z

RELEASE NOTES: none

codecov · 2023-12-01T06:17:02Z

Codecov Report

Merging #6827 (1ed10f4) into master (7935c4f) will increase coverage by 0.35%.
Report is 49 commits behind head on master.
The diff coverage is n/a.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6827      +/-   ##
==========================================
+ Coverage   83.40%   83.75%   +0.35%     
==========================================
  Files         285      287       +2     
  Lines       30879    30928      +49     
==========================================
+ Hits        25754    25904     +150     
+ Misses       4053     3963      -90     
+ Partials     1072     1061      -11

see 65 files with indirect coverage changes

dfawley · 2023-12-07T21:04:56Z

interop/client/client.go

@@ -267,103 +269,103 @@ func main() {
 	tc := testgrpc.NewTestServiceClient(conn)
 	switch *testCase {
 	case "empty_unary":
-		interop.DoEmptyUnaryCall(tc)
+		interop.DoEmptyUnaryCall(tc, ctx)


Please make ctx the first parameter according to common Go style.

dfawley · 2023-12-07T21:06:38Z

interop/test_utils.go

 	start := time.Now()
-	ctx, cancel := context.WithDeadline(context.Background(), overallDeadline)
+	ctx, cancel := context.WithDeadline(ctx, overallDeadline)


Ideally the caller would do this and not pass overallDeadline, and check ctx.Err() instead of time.Now.After(overallDeadline). If you don't mind making that change, too. Or we can do it separately if it turns out to be too much.

Thanks!

github-actions · 2023-12-13T22:45:50Z

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

Aditya-Sood · 2023-12-22T06:08:02Z

hi @dfawley, could you please reopen the PR?
I have pushed the code review changes now

dfawley

Sorry for the delay. Mostly LGTM; just one thing (inline).

dfawley · 2024-01-09T18:02:59Z

interop/test_utils.go

@@ -724,9 +724,9 @@ func doOneSoakIteration(ctx context.Context, tc testgrpc.TestServiceClient, rese
 // If resetChannel is false, then each RPC will be performed on tc. Otherwise, each RPC will be performed on a new
 // stub that is created with the provided server address and dial options.
 // TODO(mohanli-ml): Create SoakTestOptions as a parameter for this method.
-func DoSoakTest(tc testgrpc.TestServiceClient, serverAddr string, dopts []grpc.DialOption, resetChannel bool, soakIterations int, maxFailures int, soakRequestSize int, soakResponseSize int, perIterationMaxAcceptableLatency time.Duration, minTimeBetweenRPCs time.Duration, overallDeadline time.Time) {
+func DoSoakTest(ctx context.Context, cancel context.CancelFunc, tc testgrpc.TestServiceClient, serverAddr string, dopts []grpc.DialOption, resetChannel bool, soakIterations int, maxFailures int, soakRequestSize int, soakResponseSize int, perIterationMaxAcceptableLatency time.Duration, minTimeBetweenRPCs time.Duration) {


You should not pass a CancelFunc. If this function needs it, it should do ctx, cancel := context.WithCancel(ctx); defer cancel(); to avoid cancelling the parent's context.

But in this case, I don't think it needs it anyway. The defer cancel() should be done in the caller, not by this function.

Aditya-Sood · 2024-01-15T03:56:46Z

thanks @dfawley, I've pushed the change in the latest commit

one query - is the use of CancelFunc unidirectional (parent to child ctx)? or can a child be allowed to cancel the ctx for itself and its siblings?

dfawley · 2024-01-16T17:26:40Z

one query - is the use of CancelFunc unidirectional (parent to child ctx)? or can a child be allowed to cancel the ctx for itself and its siblings?

Derived contexts cannot cancel their parents/siblings and cannot be cancelled via a method on the context itself (by design).

The thing that creates a context should be the only thing able to cancel it.

dfawley · 2024-01-16T17:29:38Z

interop/client/client.go

@@ -358,10 +358,12 @@ func main() {
 		interop.DoPickFirstUnary(ctx, tc)
 		logger.Infoln("PickFirstUnary done")
 	case "rpc_soak":
-		interop.DoSoakTest(ctxWithDeadline, cancel, tc, serverAddr, opts, false /* resetChannel */, *soakIterations, *soakMaxFailures, *soakRequestSize, *soakResponseSize, time.Duration(*soakPerIterationMaxAcceptableLatencyMs)*time.Millisecond, time.Duration(*soakMinTimeMsBetweenRPCs)*time.Millisecond)
+		interop.DoSoakTest(ctxWithDeadline, tc, serverAddr, opts, false /* resetChannel */, *soakIterations, *soakMaxFailures, *soakRequestSize, *soakResponseSize, time.Duration(*soakPerIterationMaxAcceptableLatencyMs)*time.Millisecond, time.Duration(*soakMinTimeMsBetweenRPCs)*time.Millisecond)
+		cancel()


cancel should be called by a defer right after the context is created (line 271).

dfawley · 2024-01-16T17:30:21Z

interop/xds_federation/client.go

@@ -122,7 +122,8 @@ func main() {
 		go func(c clientConfig) {
 			overallDeadline := time.Now().Add(time.Duration(*soakOverallTimeoutSeconds) * time.Second)
 			ctxWithDeadline, cancel := context.WithDeadline(ctx, overallDeadline)


Similar: it's best to defer cancel() here instead (nearly always).

The main time you wouldn't defer is if the context is created inside a loop, because you don't want to stack up a very large number of defers.

Aditya-Sood · 2024-01-18T05:01:14Z

thank you for the clarifications @dfawley! I've pushed the changes to the branch

dfawley

LGTM; just a couple minor style things remain.

dfawley · 2024-01-24T18:54:33Z

interop/client/client.go

+	overallDeadline := time.Now().Add(time.Duration(*soakOverallTimeoutSeconds) * time.Second)
+	ctxWithDeadline, cancel := context.WithDeadline(ctx, overallDeadline)


Sorry, I missed this before: please use the more ergonomic WithTimeout instead:

ctxWithDeadline, cancel := context.WithTimeout(ctx, *soakOverallTimeoutSeconds * time.Second)

dfawley · 2024-01-24T18:56:16Z

interop/test_utils.go

+	outCtx := metadata.NewOutgoingContext(ctx, metadata.MD{"authorization": []string{kv["authorization"]}})
+	reply, err := tc.UnaryCall(outCtx, req)


This can just shadow or replace ctx instead of making a new thing called outCtx.

dfawley · 2024-01-24T18:56:44Z

interop/test_utils.go

 	// Testing with UnaryCall.
 	pl := ClientNewPayload(testpb.PayloadType_COMPRESSABLE, 1)
 	req := &testpb.SimpleRequest{
 		ResponseType: testpb.PayloadType_COMPRESSABLE,
 		ResponseSize: int32(1),
 		Payload:      pl,
 	}
-	ctx := metadata.NewOutgoingContext(context.Background(), customMetadata)
+	outCtx := metadata.NewOutgoingContext(ctx, customMetadata)


As above; just shadow (:=) or overwrite (=) ctx.

dfawley · 2024-01-24T18:57:41Z

interop/xds_federation/client.go

 	for i := range clients {
 		wg.Add(1)
 		go func(c clientConfig) {
-			interop.DoSoakTest(c.tc, c.uri, c.opts, resetChannel, *soakIterations, *soakMaxFailures, *soakRequestSize, *soakResponseSize, time.Duration(*soakPerIterationMaxAcceptableLatencyMs)*time.Millisecond, time.Duration(*soakMinTimeMsBetweenRPCs)*time.Millisecond, time.Now().Add(time.Duration(*soakOverallTimeoutSeconds)*time.Second))
+			overallDeadline := time.Now().Add(time.Duration(*soakOverallTimeoutSeconds) * time.Second)
+			ctxWithDeadline, cancel := context.WithDeadline(ctx, overallDeadline)


As above, please use WithTimeout instead of WithDeadline.

github-actions · 2024-01-30T20:45:43Z

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

Aditya-Sood · 2024-01-31T06:56:52Z

hi @dfawley, thanks for the feedback - I'd missed the obvious use of WithTimeout instead of WithDeadline
have pushed the review changes in new commit

dfawley

Thank you for the PR!

arvindbr8

LGTM! Thanks @Aditya-Sood

Increment: Replace context.Background() with passed ctx

4c37a22

Bug Fix: Missing ctx value

e23dd01

easwars requested a review from dfawley December 6, 2023 18:44

easwars assigned dfawley Dec 6, 2023

easwars added the Type: Testing label Dec 6, 2023

easwars added this to the 1.61 Release milestone Dec 6, 2023

dfawley reviewed Dec 7, 2023

View reviewed changes

dfawley assigned Aditya-Sood and unassigned dfawley Dec 7, 2023

dfawley added the Status: Requires Reporter Clarification label Dec 7, 2023

github-actions bot added the stale label Dec 13, 2023

github-actions bot closed this Dec 21, 2023

Code Review Changes

bc2f1b3

dfawley reopened this Dec 22, 2023

dfawley removed Status: Requires Reporter Clarification stale labels Dec 22, 2023

dfawley reviewed Jan 9, 2024

View reviewed changes

dfawley added the Status: Requires Reporter Clarification label Jan 9, 2024

Refactor: Move ctx cancel() to caller of DoSoakTest()

c2a2f88

dfawley reviewed Jan 16, 2024

View reviewed changes

Refactor: defer ctx cancel() calls

dfe089c

zasweq modified the milestones: 1.61 Release, 1.62 Release Jan 23, 2024

zasweq assigned dfawley Jan 24, 2024

zasweq unassigned Aditya-Sood Jan 24, 2024

zasweq removed the Status: Requires Reporter Clarification label Jan 24, 2024

dfawley reviewed Jan 24, 2024

View reviewed changes

dfawley assigned Aditya-Sood and unassigned dfawley Jan 24, 2024

dfawley added the Status: Requires Reporter Clarification label Jan 24, 2024

github-actions bot added stale and removed stale labels Jan 30, 2024

Code Review Changes

1ed10f4

arvindbr8 removed the Status: Requires Reporter Clarification label Jan 31, 2024

arvindbr8 assigned dfawley and unassigned Aditya-Sood Jan 31, 2024

dfawley approved these changes Jan 31, 2024

View reviewed changes

dfawley assigned arvindbr8 and unassigned dfawley Jan 31, 2024

dfawley requested a review from arvindbr8 January 31, 2024 17:43

arvindbr8 approved these changes Jan 31, 2024

View reviewed changes

arvindbr8 merged commit a3f5ed6 into grpc:master Jan 31, 2024
14 checks passed

arvindbr8 mentioned this pull request Feb 7, 2024

Remove dependency on package intended for internal testing only google.golang.org/grpc/interop open-telemetry/opentelemetry-go-contrib#4896

Closed

github-actions bot locked as resolved and limited conversation to collaborators Jul 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

interop: Replace context.Background() with passed ctx #6827

interop: Replace context.Background() with passed ctx #6827

Aditya-Sood commented Dec 1, 2023

codecov bot commented Dec 1, 2023 •

edited

Loading

dfawley Dec 7, 2023

dfawley Dec 7, 2023

github-actions bot commented Dec 13, 2023

Aditya-Sood commented Dec 22, 2023

dfawley left a comment

dfawley Jan 9, 2024

Aditya-Sood commented Jan 15, 2024

dfawley commented Jan 16, 2024

dfawley Jan 16, 2024

dfawley Jan 16, 2024

Aditya-Sood commented Jan 18, 2024

dfawley left a comment

dfawley Jan 24, 2024

dfawley Jan 24, 2024

dfawley Jan 24, 2024

dfawley Jan 24, 2024

github-actions bot commented Jan 30, 2024

Aditya-Sood commented Jan 31, 2024

dfawley left a comment

arvindbr8 left a comment

		overallDeadline := time.Now().Add(time.Duration(soakOverallTimeoutSeconds) time.Second)
		ctxWithDeadline, cancel := context.WithDeadline(ctx, overallDeadline)

		outCtx := metadata.NewOutgoingContext(ctx, metadata.MD{"authorization": []string{kv["authorization"]}})
		reply, err := tc.UnaryCall(outCtx, req)

interop: Replace context.Background() with passed ctx #6827

interop: Replace context.Background() with passed ctx #6827

Conversation

Aditya-Sood commented Dec 1, 2023

codecov bot commented Dec 1, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 13, 2023

Aditya-Sood commented Dec 22, 2023

dfawley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aditya-Sood commented Jan 15, 2024

dfawley commented Jan 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aditya-Sood commented Jan 18, 2024

dfawley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 30, 2024

Aditya-Sood commented Jan 31, 2024

dfawley left a comment

Choose a reason for hiding this comment

arvindbr8 left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 1, 2023 •

edited

Loading