Skip to content

transport: surface subsequent data when receiving non-gRPC header#8929

Open
chengxilo wants to merge 30 commits into
grpc:masterfrom
chengxilo:surface-response-body
Open

transport: surface subsequent data when receiving non-gRPC header#8929
chengxilo wants to merge 30 commits into
grpc:masterfrom
chengxilo:surface-response-body

Conversation

@chengxilo
Copy link
Copy Markdown
Contributor

@chengxilo chengxilo commented Feb 22, 2026 •
edited by easwars
Loading

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

[header]
":status", "200",
"content-type", "text/html",

[payload]
<html><body>Hello World</body></html>

Return

rpc error: code = Unknown desc = unexpected HTTP status code received from server: 200 (OK); transport: received unexpected content-type "text/html"
data: "<html><body>Hello World</body></html>"`,

RELEASE NOTES:

  • Surface data when receiving non-gRPC header, append it to the error message with this formet [error]\n data: "[data]"

@chengxilo chengxilo force-pushed the surface-response-body branch from d42d1a5 to 3dc8573 Compare February 22, 2026 21:53
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 22, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 79.16667% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.20%. Comparing base (99312fe) to head (af1df1b).
⚠️ Report is 62 commits behind head on master.

Files with missing lines Patch % Lines
internal/transport/http2_client.go 64.28% 4 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #8929      +/-   ##
==========================================
+ Coverage   83.11%   83.20%   +0.09%     
==========================================
  Files         413      418       +5     
  Lines       33135    33732     +597     
==========================================
+ Hits        27540    28068     +528     
- Misses       4190     4246      +56     
- Partials     1405     1418      +13
Files with missing lines Coverage Δ
internal/transport/client_stream.go 100.00% <100.00%> (ø)
internal/transport/http2_client.go 92.19% <64.28%> (-0.66%) ⬇️

... and 89 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@chengxilo chengxilo changed the title feat: surface response data when receiving an unexpected status code … feat: surface subsequent data when receiving non-gRPC header Feb 22, 2026
@chengxilo chengxilo changed the title feat: surface subsequent data when receiving non-gRPC header transport: surface subsequent data when receiving non-gRPC header Feb 23, 2026
@easwars easwars added Type: Behavior Change Behavior changes not categorized as bugs Area: Transport Includes HTTP/2 client/server and HTTP server handler transports and advanced transport features. labels Feb 24, 2026
easwars
easwars reviewed Feb 24, 2026
View reviewed changes
Comment thread internal/transport/client_stream.go Outdated
Comment thread internal/transport/client_stream.go Outdated
@easwars easwars assigned chengxilo and unassigned easwars Feb 24, 2026
@chengxilo chengxilo requested a review from easwars February 26, 2026 07:43
@chengxilo chengxilo marked this pull request as draft February 26, 2026 08:26
@chengxilo chengxilo marked this pull request as ready for review February 26, 2026 08:46
@chengxilo
Copy link
Copy Markdown
Contributor Author

chengxilo commented Feb 26, 2026 •
edited
Loading

Sorry for request a review while a data race detected 🥲. It's solved now.

To clarify my previous commit:

@easwars easwars self-assigned this Feb 26, 2026
@easwars easwars added this to the 1.80 Release milestone Feb 27, 2026
@mbissa mbissa modified the milestones: 1.80 Release, 1.81 Release Mar 6, 2026
@chengxilo chengxilo force-pushed the surface-response-body branch from f3fa621 to df12e44 Compare April 25, 2026 02:22
@chengxilo
Copy link
Copy Markdown
Contributor Author

chengxilo commented Apr 25, 2026 •
edited
Loading

  • Add a new field in ClientStream, let's maybe call it nonGPRCStatus and protect it with collectingMu (√)
  • In startNonGRPCDataCollection, set the above newly added field (√)
  • Do not close the headerChan here (√)

I followed most of the suggestion but diverged on one point: instead of adding a closeStreamWithNonGRPCStatus method on ClientStream that finalizes the status and then calls t.closeStream, I inlined the finalization logic directly in closeStream. I added some comment to clarify it so I think it would be ok.... perhaps.

  • In handleData, when we have read enough of the non-grpc data and want to close the stream, instead of calling the closeStreamWithNonGRPCStatus on the transport:

    • call a new method on the ClientStream, maybe call it closeStreamWithNonGRPCStatus or something else

    • this new method will

      • grab collectionMu
      • it will update nonGPRCStatus with non-grpc data read from the stream
      • it will then call t.closeStream and pass it the nonGPRCStatus
      • this will result in the stream closing cleanly, while respecting the existing synchronization guarantees about access to the status field
      • this will also mean that we will no longer need finalizeNonGRPCDataCollectionLocked method

The reason is that closeStream can be called from multiple paths, not just from handleData when non-gRPC data collection completes, but also from context cancellation, transport errors, etc. When closeStream is called for these reasons, it will still finalize the collected non-gRPC data into the status and return to the user.

The race on the status field between Header() readers and closeStream writers is addressed by not closing headerChan in operateHeaders because of non-GRPC header anymore. Header() now only returns after closeStream has finished writing s.status, so no lock is needed in Header().

Also I removed collecting, as whether nonGRPCStatus is nil or not can tell us whether we are collecting the non-gRPC data or not.

@chengxilo chengxilo requested a review from easwars April 25, 2026 02:38
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 2, 2026

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.

@github-actions github-actions Bot added the stale label May 2, 2026
@easwars easwars assigned easwars and arjan-bal and unassigned chengxilo May 4, 2026
easwars
easwars approved these changes May 4, 2026
View reviewed changes
Comment thread test/end2end_test.go Outdated
Comment thread test/end2end_test.go Outdated
@easwars easwars removed their assignment May 4, 2026
@easwars
Copy link
Copy Markdown
Contributor

easwars commented May 4, 2026

@arjan-bal : Over to you for second review.

arjan-bal
arjan-bal reviewed May 14, 2026
View reviewed changes
Comment thread internal/transport/client_stream.go Outdated
@arjan-bal arjan-bal assigned chengxilo and unassigned arjan-bal May 14, 2026
@chengxilo chengxilo requested a review from arjan-bal May 28, 2026 07:31
@chengxilo chengxilo force-pushed the surface-response-body branch 2 times, most recently from 5717c78 to 3db4320 Compare May 28, 2026 08:23
@chengxilo chengxilo force-pushed the surface-response-body branch from 3db4320 to f81c45a Compare May 28, 2026 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@easwars easwars easwars approved these changes

@arjan-bal arjan-bal Awaiting requested review from arjan-bal

Assignees

@chengxilo chengxilo

Labels

Area: Transport Includes HTTP/2 client/server and HTTP server handler transports and advanced transport features. Type: Behavior Change Behavior changes not categorized as bugs

Projects

None yet

Milestone

1.82 Release

Development

Successfully merging this pull request may close these issues.

Surface response body when receiving an unexpected status code and content-type

5 participants

@chengxilo @Pranjali-2501 @easwars @arjan-bal @mbissa