benchmark/client: add context for cancellation #8614
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8614 +/- ##
==========================================
- Coverage 79.45% 79.39% -0.06%
==========================================
Files 415 415
Lines 41339 41354 +15
==========================================
- Hits 32844 32833 -11
- Misses 6621 6622 +1
- Partials 1874 1899 +25 🚀 New features to boost your workflow:
|
|
Could you please add "Type: Internal Cleanup" label? Thanks |
benchmark/worker/benchmark_client.go
Outdated
| select { | ||
| case <-bc.stop: | ||
| if bc.ctx.Err() != nil { | ||
| return |
There was a problem hiding this comment.
Can we change it to return bc.ctx.Err() and change function unaryLoop to return error?
There was a problem hiding this comment.
I believe we can, but error is happening inside a goroutine and I'm not quite sure how to implement handling. Could you suggest an approach?
There was a problem hiding this comment.
Apologies for the delayed review and thanks for the contribution!
A few main points:
- We should pass the context though function params.
- We should try to derive contexts from other contexts as much as possible instead of using context.Background.
- We can avoid a lot of the
ctx.Err()checks by letting RPCs fail and handling the case when the error had a status of CANCELLED.
benchmark/worker/benchmark_client.go
Outdated
| lockingHistograms: make([]lockingHistogram, rpcCountPerConn*len(conns)), | ||
|
|
||
| stop: make(chan bool), | ||
| ctx: ctx, |
There was a problem hiding this comment.
Could we pass the context as an argument here instead of storing it in the struct? The standard practice in Go is to pass context.Context as the first parameter, which helps make cancellation and deadlines clearer across API boundaries. The Go team wrote a post about it here: https://go.dev/blog/context-and-structs
benchmark/benchmark.go
Outdated
| } | ||
|
|
||
| // DoUnaryCallWithContext performs a unary RPC with propagated context and given stub and request and response sizes. | ||
| func DoUnaryCallWithContext(ctx context.Context, tc testgrpc.BenchmarkServiceClient, reqSize, respSize int) error { |
There was a problem hiding this comment.
We can change the existing DoUnaryCall function to accept a context. Since the benchmarking client is a binary (not a library), we don't need to be concerned too much about maintaining existing function signatures.
benchmark/worker/benchmark_client.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func startBenchmarkClient(config *testpb.ClientConfig) (*benchmarkClient, error) { |
There was a problem hiding this comment.
Can we change the startBenchmarkClient to accept a context? We can create the context in main.go we can derive a new context from the the stream context:
grpc-go/benchmark/worker/main.go
Line 166 in 8110884
// Using the stream context instead of context.Background()
ctx, cancel := context.WithCancel(stream.Context())When the client is to be closed, the context will be cancelled just before that.
grpc-go/benchmark/worker/main.go
Lines 142 to 148 in 8110884
benchmark/worker/benchmark_client.go
Outdated
| start := time.Now() | ||
| if err := benchmark.DoUnaryCall(client, reqSize, respSize); err != nil { | ||
| if err := benchmark.DoUnaryCallWithContext(bc.ctx, client, reqSize, respSize); err != nil { | ||
| if status.Code(err) == codes.Canceled || errors.Is(err, context.Canceled) { |
There was a problem hiding this comment.
I don't think we need the second part of the condition (|| errors.Is(err, context.Canceled). gRPC should always be returning an error which can be converted to a gRPC status. Please do let me know if this is not the case, it may be a bug.
benchmark/worker/benchmark_client.go
Outdated
| if bc.ctx.Err() != nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
We can avoid the context check since the stream context will be cancelled when the client is being closed, automatically failing the stream.
benchmark/worker/benchmark_client.go
Outdated
| if bc.ctx.Err() != nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
We can avoid the context check since the stream context will be cancelled when the client is being closed, automatically failing the stream.
benchmark/worker/benchmark_client.go
Outdated
| if bc.ctx.Err() != nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
Same here, we can avoid the context check since the stream context will be cancelled when the client is being closed, automatically failing the stream.
benchmark/worker/benchmark_client.go
Outdated
| if bc.ctx.Err() != nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
Same here, we can avoid the context check since the stream context will be cancelled when the client is being closed, automatically failing the stream.
benchmark/worker/benchmark_client.go
Outdated
| if bc.ctx.Err() != nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
Same here, we can avoid the context check since the stream context will be cancelled when the client is being closed, automatically failing the stream.
|
Thank you very much for the review, it is very helpful! |
| bc.lockingHistograms[idx].histogram = stats.NewHistogram(bc.histogramOptions) | ||
| if poissonLambda == nil { // Closed loop. | ||
| if stream.Context().Err() != nil { | ||
| return | ||
| } | ||
| // Start goroutine on the created mutex and histogram. | ||
| go func(idx int) { | ||
| // TODO: do warm up if necessary. |
There was a problem hiding this comment.
You said
We usually do the check just before performing an asynchronous operation
so I decided to do it like that. If it is not the appropriate way, please tell me
3 participants
Fixes: #8596
Added cancellation by context in benchmark/client instead of custom channel approach. Not sure about returning
ctx.Err()(as mentioned in the original issue under point 4), so please reviewRELEASE NOTES: None