Skip to content

grpc: place defaultStreamInterceptor after user interceptors - #9404

Open
easwars wants to merge 4 commits into
grpc:masterfrom
easwars:change_default_stream_interceptor_position
Open

grpc: place defaultStreamInterceptor after user interceptors#9404
easwars wants to merge 4 commits into
grpc:masterfrom
easwars:change_default_stream_interceptor_position

Conversation

@easwars

@easwars easwars commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

The defaultStreamInterceptor was introduced in #9226.

This change places the defaultStreamInterceptor which performs certain common logic for all non-unary RPCs to live after the user interceptors and before the xDS filters. This ensures that the work of the defaultStreamInterceptor stays invisible to the user interceptors. Specifically, it calls RecvMsg twice for non server-streaming RPCs and calls CloseSend after sending the only message for non client-streaming RPCs.

RELEASE NOTES: none

The `defaultStreamInterceptor` was introduced in grpc#9226.

This change places the `defaultStreamInterceptor` which performs certain
common logic for all non-unary RPCs to live after the user interceptors
and before the xDS filters. This ensures that the work of the
`defaultStreamInterceptor` stays invisible to the user interceptors.
Specifically, it calls `RecvMsg` twice for non server-streaming RPCs and
calls `CloseSend` after sending the only message for non
client-streaming RPCs.
@easwars

easwars commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

/gemini review

@easwars easwars added the Type: Internal Cleanup Refactors, etc label Sep 9, 2026
@easwars easwars added this to the 1.84 Release milestone Sep 9, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request modifies the order of stream client interceptors in clientconn.go so that defaultStreamInterceptor is appended to the end of the interceptors slice rather than prepended. It also updates existing tests and adds a new end-to-end test (TestDefaultStreamInterceptor_InteractionWithXDSFilters) to verify that the internal behaviors of the default stream interceptor are not visible to user-defined interceptors and interact correctly with xDS filters. The feedback highlights several misleading comments in the new test assertions regarding client-side terminology and accumulated message counts, and suggests checking the ignored error returned by CloseSend().

Comment thread test/xds/xds_client_filter_e2e_test.go Outdated
Comment thread test/xds/xds_client_filter_e2e_test.go Outdated
Comment thread test/xds/xds_client_filter_e2e_test.go Outdated
Comment thread test/xds/xds_client_filter_e2e_test.go Outdated
Comment thread test/xds/xds_client_filter_e2e_test.go Outdated
@codecov

codecov Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.47%. Comparing base (5c58cee) to head (ebe647c).
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9404      +/-   ##
==========================================
- Coverage   87.51%   87.47%   -0.05%     
==========================================
  Files         429      430       +1     
  Lines       30643    30675      +32     
==========================================
+ Hits        26817    26832      +15     
- Misses       3826     3842      +16     
- Partials        0        1       +1     
Files with missing lines Coverage Δ
clientconn.go 92.61% <100.00%> (+0.26%) ⬆️
stream.go 88.31% <ø> (ø)

... and 24 files with indirect coverage changes

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

@easwars

easwars commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request modifies the client connection interceptor chaining to append the defaultStreamInterceptor to the end of the interceptors slice instead of prepending it. It updates existing tests in stream_test.go to verify that the default stream interceptor's automatic CloseSend and RecvMsg behaviors are not visible to user-defined interceptors. Additionally, it introduces a new end-to-end test in test/xds/xds_client_filter_e2e_test.go to verify the interaction between the default stream interceptor and xDS filters across various RPC types, and extends the tracking filter builder in test/xds/xds_server_filter_state_retention_test.go to support client-side filter interception. There are no review comments, and I have no additional feedback to provide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants