lizining1231 commented on issue #3694:
URL: https://github.com/apache/dubbo-go/issues/3694#issuecomment-5398185747
> Thanks for the proposal. The direction is reasonable, but I would not take
it as one broad change yet.
>
> The main risk is concurrency: `StreamingClientConn` allows `Send`,
`RequestHeader`, and `CloseRequest` to race with each other, so any replacement
for `io.Pipe` needs an explicit write-side snapshot/lock boundary, not only a
response-side `Read`/`Close` guard. Otherwise `CloseRequest` can observe a
partially written request body.
>
> The performance claim also needs a tighter proof. The current unary
marshaler still allocates via `codec.Marshal` before anything reaches the
transport, so the claimed copy reduction is not yet established. I would want
an alloc benchmark, not just a transport rewrite, before calling that part
solved.
>
> There is also a body lifetime issue to pin down. `net/http.Client.Do` may
close request bodies asynchronously after it returns, so buffer reuse needs a
precise rule across transport errors, redirects/retries, and context cancel.
Without that, pool return can become racy.
>
> My suggestion is to split this into two steps: first a unary-only fast
path behind a default-off flag that preserves current behavior for gRPC and
streaming, then a separate pass for marshal/allocation reduction with race and
alloc benchmarks.
Thank you for the detailed review. My responses point by point are as
follows:
1. **Regarding the overly large change scope: split into two medium-sized
PRs**
Adjust the change scope by splitting this round of work into two PRs. The
first PR will only implement the unary fast path (`unaryFastPathCall`), with
the feature flag off by default. The gRPC protocol and streaming path remain
unchanged (`duplexHTTPCall`), the marshaler/unmarshaler and bufferPool are
fully reused, and only the send path itself is replaced. Allocation
optimizations (including the `codec.Marshal` layer) are explicitly deferred to
the second phase and will not be mixed into the first step.
2. **Regarding the write concurrency boundary issue: add fixes**
Add a shared write lock for `Write` and `CloseWrite`. The
`StreamingClientConn` interface semantically allows concurrent
`Send`/`RequestHeader`/`CloseRequest`. The unary normal call sequence is serial
(the request body `Write` completes fully before `CloseWrite` triggers sending,
and `sendOnce` guarantees `makeRequest` executes exactly once). However, to
satisfy the interface's concurrency semantics, we will add explicit
synchronization on the write side in the implementation: `Write` and
`CloseWrite` share a write lock (taking a snapshot of the request body),
ensuring that `CloseRequest` does not observe a partially written request body
under concurrency. This change will be submitted with the code, along with
`-race` concurrency tests (exactly-once send, reject writes after failure).
3. **Regarding the missing allocation test data: supplement here**
Attached are simple in-stack A/B test results within the protocol
(httptest h2c server, both A and B sides share
marshaler/unmarshaler/bufferPool, only the send path differs, count=3 taking
the median):
| payload | duplex allocs/op | fastpath allocs/op | allocs change | B/op
change |
|---------|------------------|--------------------|---------------|-------------|
| 128B | 141 | 140 | ≈0% | -10%
|
| 1024B | 139 | 139 | (flat) | -13%
|
| 16384B | 147 | 145 | -1% | -9%
|
| 2MiB | 372 | 324 | -13% | -4%
|
The situation is as stated — in-stack allocs are essentially flat. The
protocol-layer duplex and fastpath share bufferPool and marshaler. The
allocation reduction from the fast path mainly comes from the `io.Pipe` itself,
the per-request goroutine stack, and the body wrapper layer
(`io.NopCloser+bytes.Reader` → zero-allocation `unaryRequestBody`);
`codec.Marshal` allocations indeed occur before reaching the transport and are
not within the scope of this phase's reduction.
4. **Regarding the body lifecycle and pool return: supplement the boundary
conditions**
Clarify the design for this risk and supplement the boundary conditions.
Buffer return is not placed in `makeRequest` or `CloseRead`, but is delegated
to the transport's exactly-once callback when the request body `Close()` is
called. You correctly pointed out that after `net/http.Client.Do` returns, the
request body may be closed asynchronously — indeed, `x/net/http2` calls
`reqBody.Close()` via an independent goroutine in `closeReqBodyLocked`
(transport.go L381), and the transport's background goroutine may still be
reading the request body when `Do` returns. Therefore, return cannot occur at
the `Do` return site. Instead, it is placed in the transport's `Close()`
callback on the request body (invoked exactly once either after normal
completion of reading, or when the server responds early/aborts via
`closeReqBodyLocked`), and the buffer is returned in that callback.
Additionally, the following boundary conditions/tests are supplemented:
- Normal read completion: exactly one return, no double return;
- Server early response (non-2xx abort): the transport interrupts request
body writing and calls `Close`, the buffer is still returned without leakage;
- Redirect/retry: `net/http` closes the request body on redirect, which
also falls into the `Close` callback;
- Context cancellation: the transport aborts request body writing and
closes the body, same path as above;
- Return and in-flight `Read` are serialized by the `unaryRequestBody`
mutex lock to prevent data races and cross-request contamination.
Accompanying `-race` tests: abort return, concurrent read-close,
read-after-close (use-after-return), exactly-once return.
--
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]
