lizining1231 opened a new pull request, #3749: URL: https://github.com/apache/dubbo-go/pull/3749
### Description Fixes # 3768 The main change is to enable Triple streaming write buffering by default. This makes `WithWriteBuffering()` redundant because it only sets the default value. `WithUnaryFastPath()` became redundant when the unary fast path was enabled by default, so this change also removes it: | Option | When it became redundant | Why it no longer changes the configuration | | --- | --- | --- | | `WithUnaryFastPath()` | After `clientConfig.UnaryFastPath` was enabled by default in #3694 | `applyToClient` still sets it to `true` | | `WithWriteBuffering()` | When this change enables `clientConfig.WriteBuffering` by default | `applyToClient` still sets it to `true` | Keeping `WithWriteBuffering()` could mislead callers into thinking they must explicitly enable write buffering, when it is already enabled by default. Both ineffective enable options should therefore be removed. The options for disabling these features must remain available. `WithoutUnaryFastPath()` already exists; this change adds `WithoutWriteBuffering()` so callers can disable write buffering when needed. In addition to changing the default and removing redundant options, this change must fix two issues in the buffering layer: a zero-length write must start the request, and `Send` must return a cancellation error for a small message when the context has been canceled. Both cases need regression tests. ### Relevant code The following table identifies the relevant code on `develop` before this change. | Location | Purpose | | --- | --- | | [`option.go#L73-L91`](https://github.com/apache/dubbo-go/blob/develop/protocol/triple/triple_protocol/option.go#L73-L91) | The redundant `WithUnaryFastPath()` constructor, `unaryFastPathOption` struct, and its `applyToClient` method | | [`option.go#L93-L104`](https://github.com/apache/dubbo-go/blob/develop/protocol/triple/triple_protocol/option.go#L93-L104) | `WithoutUnaryFastPath()`, which disables the unary fast path | | [`option.go#L106-L142`](https://github.com/apache/dubbo-go/blob/develop/protocol/triple/triple_protocol/option.go#L106-L142) | The redundant `WithWriteBuffering()` constructor, `writeBufferingOption` struct, and its `applyToClient` method | | [`triple_protocol/client.go#L245-L254`](https://github.com/apache/dubbo-go/blob/develop/protocol/triple/triple_protocol/client.go#L245-L254) | The client constructor, which sets `UnaryFastPath: true` while leaving `WriteBuffering` at its zero value | | [`protocol/triple/client.go#L184-L195`](https://github.com/apache/dubbo-go/blob/develop/protocol/triple/client.go#L184-L195) | The framework code that converts configuration into `ClientOption` values, handling nil, `true`, and `false` separately | | [`global/triple_config.go#L52-L57`](https://github.com/apache/dubbo-go/blob/develop/global/triple_config.go#L52-L57) | `TripleConfig.UnaryFastPath`, which can serve as a reference when adding the `WriteBuffering` field | ### buffering defects exposed by the new default Write buffering previously had to be enabled manually, so most tests did not exercise `streamBufferWriter`. Once it is enabled by default, issues with zero-length writes and context cancellation directly affect existing streaming calls. #### 1. Zero-length writes are swallowed `Send(nil)` eventually makes a zero-length `Write` call. Without buffering, `duplexHTTPCall.Write` starts the HTTP request even when there are no bytes to write. With buffering, writing zero bytes leaves the buffer empty, and `flushLocked` returns immediately because `buf.Len() == 0`. The request is therefore never sent. `BlockUntilResponseReady` then waits indefinitely for `responseReady`; it does not observe context cancellation, so even the deadline cannot end the wait. As a result, a stream that sends only a nil message hangs and causes the entire package test run to time out. Fix: Pass zero-length writes directly to the underlying writer without buffering them. Leave any existing nonempty buffered data in place, and return errors from the underlying write to the caller. #### 2. Small messages bypass the context check `duplexHTTPCall.Write` checks the context before writing. If the context has been canceled, it returns `CodeCanceled` and records the error so that subsequent reads also observe the failure. With buffering enabled, however, a small message is written only to the in-memory buffer and bypasses this check, so `Send` may incorrectly report success. The existing `TestContextError` sends only a nil message; after the zero-length write fix, that case goes directly to the underlying writer and cannot independently cover this issue. `TestServer/.../cumsum_cancel_before_send` uses a nonempty message and can detect the issue in the gRPC encoding path. Fix: Have `streamBufferWriter` call `preWriteChecker` before accepting data, with `duplexHTTPCall` implementing the check. When the context is valid, the check should only inspect its state: the request should still start, and its headers should still be sent, on the first flush. When the context has been canceled, follow the direct-write path: start the request, record the error, and return `CodeCanceled`. Subsequent reads should also return the cancellation error. ## Proposed changes Enable `WriteBuffering` by default, remove the two redundant `With*` constructors, and add `WithoutWriteBuffering()`. Add a `WriteBuffering` field to the framework configuration so users can disable buffering through configuration. The specific changes are: #### Enable write buffering by default - Set `clientConfig.WriteBuffering` to `true` by default in the Triple client constructor. - Add `WriteBuffering *bool` to `TripleConfig`, enable it by default, and handle the field in `DefaultTripleConfig` and `Clone`. - Change the streaming A/B benchmark’s `-buffering` default to `true`; have the baseline group explicitly use `WithoutWriteBuffering()`. #### Clean up client options - Once write buffering is enabled by default, remove `WithWriteBuffering()`, which only repeats the default, and retain `WithoutWriteBuffering()` for explicitly disabling it. - Also remove the already redundant `WithUnaryFastPath()`, retain `WithoutUnaryFastPath()`, and update the relevant comments. - Have the framework client pass a disable option only when the corresponding configuration value is explicitly `false`. Remove references to the deleted options from tests and benchmarks. #### Fix defects exposed by the new default - Pass zero-length writes directly to the underlying writer so they start the HTTP request, while preserving any data already in the buffer. - Check the context before the buffering layer accepts a write. Have `duplexHTTPCall.checkBeforeWrite()` handle cancellation so that both the write and subsequent reads return `CodeCanceled`. - Add targeted regression tests for both issues and run the affected streaming integration tests. #### Update existing tests and benchmarks - Add separate tests to confirm that write buffering and the unary fast path are enabled by default. - Remove unnecessary enable options from streaming test helpers and unary benchmarks. Keep the two `CloseResponse` tests on the unbuffered write path because they require messages sent during setup to reach the server before `Receive` is called. ### Tests #### New tests | Test | What it verifies | | --- | --- | | `TestUnaryFastPathEnabledByDefault` | The unary fast path remains enabled by default after the redundant enable option is removed. | | `TestWriteBufferingEnabledByDefault` | Streaming write buffering is enabled by default. | | `TestStreamBufferWriterForwardsZeroLengthWrite` | A zero-length write reaches the underlying writer directly; a subsequent small message remains buffered until `Flush` is called. | | `TestStreamBufferWriterChecksContextBeforeBuffering` | A small message does not start the request early while the context is valid; a write after cancellation and subsequent reads both return `CodeCanceled`. | ### Updated tests | Test | What it verifies | | --- | --- | | `TestContextError` | Adds a nonempty small-message case to cover the context cancellation check on the buffered path. | | `TestBidiStreamCloseResponseDoesNotDrainResponse` | Explicitly uses the unbuffered write path in the existing close-behavior test. | | `TestBidiStreamCloseResponseAfterServerStopsReading` | Explicitly uses the unbuffered write path in the existing close-behavior test. | ### Checklist - [x] I confirm the target branch is `develop` - [x] I have run `make fmt` to format my code - [x] I have run `make test` to run local tests - [x] I have added tests that prove my fix is effective or that my feature works -- 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]
