lizining1231 opened a new issue, #3675:
URL: https://github.com/apache/dubbo-go/issues/3675
### Summary
In `tools/benchmark/client/engine/engine.go`, `Stop()` cancels the parent
context **before** draining in-flight requests, causing exactly `concurrency`
requests to be recorded as `context canceled` failures on every benchmark
teardown. In low-QPS scenarios (1MiB payload), the failure rate reaches 0.86%,
distorting the success-rate metric.
### Affected Locations
| Package | Related Source
|
| ----------------------------- |
------------------------------------------------------------------------------------------------------------------------------------------------------
|
| tools/benchmark/client/engine |
<https://github.com/apache/dubbo-go/blob/develop/tools/benchmark/client/engine/engine.go#L114-L121>
|
### Reproduction
```bash
# 1MiB payload + 10s short test to amplify the effect
cd tools/benchmark && \
bash scripts/run_single.sh dubbo-go 1048576 protobuf none 50 unary \
2>&1 | tee data/logs/reproduce_1mib.log
# Check the failure count
grep -c 'lastErr=canceled: context canceled' data/logs/reproduce_1mib.log
# Observe: Failure equals exactly 50 (concurrency), logs flooded with
lastErr=canceled: context canceled
```
```bash
========================================
Benchmark Report
========================================
QPS: 116.43
Success Rate: 99.29%
Total Requests: 7044
Success: 6994
Failure: 50
lizining@Y:~/projects/dubbo-go/tools/benchmark$ grep -c 'lastErr=canceled:
context canceled' data/logs/reproduce_1mib.log
50
```
<br />
### Root Cause
```go
//
https://github.com/apache/dubbo-go/blob/develop/tools/benchmark/client/engine/engine.go#L114-L121
func (e *Engine) Stop() {
e.stopOnce.Do(func() {
close(e.stopChan) // ① prevent new requests
e.cancel() // ② cancel parent ctx
e.wg.Wait() // ③ wait for workers
})
}
```
50 workers each have one in-flight request executing. Step ② cancels all 50
child contexts, each returning `context canceled`. The worker at L108 records
the error via `e.metricsCollector.Record(duration, err)`. With 128B (260K total
requests), 50 failures = 0.018%; with 1MiB (5,800 total requests), 50 failures
= 0.86%.
### Proposed Fix
Swap `e.cancel()` and `e.wg.Wait()` — drain in-flight requests before
canceling:
```go
func (e *Engine) Stop() {
e.stopOnce.Do(func() {
close(e.stopChan) // ① prevent new requests
e.wg.Wait() // ② wait for all in-flight requests to complete
naturally
e.cancel() // ③ cleanup (no workers remaining)
logger.Info("[INFO] Benchmark completed")
})
}
```
- No deadlock risk: `benchmarkFunc(ctx)` has a `requestTimeout` safety bound
(default 30s)
- No race: `stopOnce.Do` guards against double execution
- Test duration may extend by `max(in-flight request duration)` (\~1s for
1MiB, negligible for 128B), which is correct behavior, benchmarks should
measure full request lifecycles, not truncate the last batch
### Verification
```bash
# Before fix: failure = 50
bash scripts/run_single.sh dubbo-go 1048576 protobuf none 50 unary 10s
# Expected: Failure: 50, Success Rate: ~99.27%
# After fix: failure = 0
bash scripts/run_single.sh dubbo-go 1048576 protobuf none 50 unary 10s
# Expected: Failure: 0, Success Rate: 100.00%
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
