transport: surface subsequent data when receiving non-gRPC header#8929
transport: surface subsequent data when receiving non-gRPC header#8929chengxilo wants to merge 26 commits into
Conversation
chengxilo
force-pushed
the
surface-response-body
branch
from
d42d1a5 to
3dc8573
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8929 +/- ##
==========================================
+ Coverage 83.11% 83.14% +0.02%
==========================================
Files 413 413
Lines 33135 33524 +389
==========================================
+ Hits 27540 27872 +332
- Misses 4190 4233 +43
- Partials 1405 1419 +14
🚀 New features to boost your workflow:
|
chengxilo
changed the title
…) instead to simplify the code.
chengxilo
changed the title
easwars
added
Type: Behavior Change
|
Sorry for request a review while a data race detected 🥲. It's solved now. To clarify my previous commit:
|
…oseStream Replace the `collecting` bool and `finalizeNonGRPCDataCollectionLocked` method with a `nonGRPCStatus` field on ClientStream. The non-gRPC status is now stored separately from the stream's `status` field and finalized inline in closeStream, which eliminates the race on `status` between Header() readers and closeStream writers.
…esponses Do not close headerChan in operateHeaders when a non-gRPC response is received. Instead, let closeStream close it when the stream finishes. This ensures Header() only returns after the status is finalized, eliminating the need for collectionMu in Header().
…er cases The original TestHeaderChanClosedAfterReceivingAnInvalidHeader was actually testing the non-gRPC response path (!isGRPC), not the malformed header path (headerError). Split into two tests: - TestHeaderChanClosedAfterReceivingNonGRPCResponse: verifies headerChan closes when the stream closes (via context timeout) - TestHeaderChanClosedAfterReceivingAnInvalidHeader: verifies headerChan closes immediately via closeStream on the headerError path
chengxilo
force-pushed
the
surface-response-body
branch
from
f3fa621 to
df12e44
Compare
I followed most of the suggestion but diverged on one point: instead of adding a
The reason is that closeStream can be called from multiple paths, not just from The race on the Also I removed |
|
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. |
| wantCode: codes.Unavailable, | ||
| wantErr: `rpc error: code = Unavailable desc = unexpected HTTP status code received from server: 502 (Bad Gateway); malformed header: missing HTTP content-type |
There was a problem hiding this comment.
Nit: Since we are also checking the returned status with wantCode, we can skip the rpc error: code = Unavailable desc part in wantErr, and instead of performing a strict equality check err.Error() != test.wantErr, you could do a strings.Contains()
There was a problem hiding this comment.
I skip the rpc error: code = Unavailable desc part in wantErr now.
However I did't use strings.Contains() because it's too loose for this test case:
{
name: "non-gRPC content-type with bytes payload length more than nonGRPCDataMaxLen",
responses: []httpServerResponse{
{
headers: [][]string{
{
":status", "200",
"content-type", "text/html",
},
},
payload: bytes.Repeat([]byte("a"), nonGRPCDataMaxLen+1),
},
},
wantCode: codes.Unknown,
wantErr: `rpc error: code = Unknown desc = unexpected HTTP status code received from server: 200 (OK); transport: received unexpected content-type "text/html"
data: ` + strconv.Quote(strings.Repeat("a", nonGRPCDataMaxLen)),
},When the payload exceeds nonGRPCDataMaxLen, we expect the data to be truncated to exactly 1024 bytes. But strings.Contains() would pass even if truncation were broken, since a string of 1024 "a"s is always a substring of 1025 "a"s, the test would silently pass with untruncated data.
|
@arjan-bal : Over to you for second review. |
| headerValid bool | ||
| headerValid bool | ||
|
|
||
| collectionMu sync.Mutex // protects nonGRPCStatus and nonGRPCDataBuf during the non-gRPC data collection lifecycle. |
There was a problem hiding this comment.
I think we may not need the mutex here. The three places where the nonGRPC fields are accessed are:
operateHeadershandleDatacloseStream
Methods 1 and 2 are called serially by the goroutine running the controlbuf loop.
Method 3 reads the fields to overrides the arguments passed to it. This seems undesirable because it ignores the status passed by the caller.
The way to gracefully close streams in HTTP/2 is by using the "end of stream" (EOS) flag in the frame header. Since this flag is only present in HEADERS and DATA frames, we should have operateHeaders and handleData check the nonGRPCStatus, call closeStream with the resolved status, and remove the extra logic from closeStream. This approach should also eliminate the need for the mutex.
5 participants
Fixes #7406
If content-type is not grpc, we read the next data frames till 1kb or endStream, and append the subsequent data to error message.
Example:
When receiving
Return
RELEASE NOTES:
[error]\n data: "[data]"