Skip to content

Commit bb5c6ea

Browse files
easwarseshitachandwani
authored andcommitted
grpc: place defaultStreamInterceptor after user interceptors (grpc#9404)
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. RELEASE NOTES: none --------- Co-authored-by: eshitachandwani <emchandwani@google.com> (cherry picked from commit 2fbc883)
1 parent c31db30 commit bb5c6ea

5 files changed

Lines changed: 340 additions & 29 deletions

File tree

clientconn.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -553,7 +553,7 @@ func chainStreamClientInterceptors(cc *ClientConn) {
553553
if cc.dopts.streamInt != nil {
554554
interceptors = append([]StreamClientInterceptor{cc.dopts.streamInt}, interceptors...)
555555
}
556-
interceptors = append([]StreamClientInterceptor{defaultStreamInterceptor}, interceptors...)
556+
interceptors = append(interceptors, defaultStreamInterceptor)
557557
var chainedInt StreamClientInterceptor
558558
if len(interceptors) == 0 {
559559
chainedInt = nil

stream.go

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,18 @@ type ClientStream interface {
153153
// RecvMsg parities based on the nature of stream.
154154
type clientStreamWrapper struct {
155155
ClientStream
156-
desc *StreamDesc
156+
desc *StreamDesc
157+
closeSendCalled atomic.Bool
158+
}
159+
160+
// CloseSend closes the send direction of the stream. The implementation ensures
161+
// that CloseSend is only called once on the underlying ClientStream, even if
162+
// CloseSend is called multiple times on the wrapper.
163+
func (w *clientStreamWrapper) CloseSend() error {
164+
if w.closeSendCalled.Swap(true) {
165+
return nil
166+
}
167+
return w.ClientStream.CloseSend()
157168
}
158169

159170
// SendMsg sends message m across the stream. For RPCs where client can call
@@ -177,11 +188,13 @@ func (w *clientStreamWrapper) SendMsg(m any) error {
177188
if err != nil {
178189
return err
179190
}
180-
// CloseSend is needed because in some scenarios (e.g., xDS), the same
181-
// interceptors are used to process both unary and streaming RPCs. Calling
182-
// CloseSend signals to those interceptors that no more messages are on the
183-
// way.
184-
if err := w.ClientStream.CloseSend(); err != nil && err != io.EOF {
191+
// In some scenarios (e.g., xDS), the same interceptors process both unary and
192+
// streaming RPCs, relying on CloseSend to signal that no more messages are on
193+
// the way. Although protobuf-generated stubs already invoke CloseSend for
194+
// server-streaming RPCs, it is explicitly called here to ensure downstream
195+
// interceptors are also notified when callers interact with the ClientStream
196+
// API directly.
197+
if err := w.CloseSend(); err != nil && err != io.EOF {
185198
return err
186199
}
187200
return nil

stream_test.go

Lines changed: 22 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -151,8 +151,8 @@ func (s) TestUnaryClient_ServerStreamingMismatch(t *testing.T) {
151151
// CloseSend hooks across downstream interceptors.
152152
type interceptorStream struct {
153153
grpc.ClientStream
154-
recvMsgCount int
155-
closeSend bool
154+
recvMsgCount int
155+
closeSendCount int
156156
}
157157

158158
func (s *interceptorStream) RecvMsg(m any) error {
@@ -161,14 +161,15 @@ func (s *interceptorStream) RecvMsg(m any) error {
161161
}
162162

163163
func (s *interceptorStream) CloseSend() error {
164-
s.closeSend = true
164+
s.closeSendCount++
165165
return s.ClientStream.CloseSend()
166166
}
167167

168-
// TestDefaultStreamInterceptor verifies that defaultStreamInterceptor
169-
// automatically triggers CloseSend on non-client-streaming RPCs right after
170-
// SendMsg, and calls RecvMsg a second time on non-server-streaming RPCs to
171-
// consume trailers and io.EOF.
168+
// TestDefaultStreamInterceptor verifies that defaultStreamInterceptor's
169+
// behavior of automatically triggering CloseSend on non-client-streaming RPCs
170+
// right after SendMsg, and calling RecvMsg a second time on
171+
// non-server-streaming RPCs to consume trailers and io.EOF, are not visible to
172+
// user-defined interceptors.
172173
func (s) TestDefaultStreamInterceptor(t *testing.T) {
173174
var iStream *interceptorStream
174175
// Define a client-side stream interceptor that wraps the ClientStream to
@@ -207,10 +208,11 @@ func (s) TestDefaultStreamInterceptor(t *testing.T) {
207208
ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
208209
defer cancel()
209210

210-
// Make a client-streaming RPC. When CloseAndRecv invokes RecvMsg once to get
211-
// the single reply message on a non-server-streaming RPC,
211+
// Make a client-streaming RPC. When CloseAndRecv invokes RecvMsg once to
212+
// get the single reply message on a non-server-streaming RPC,
212213
// defaultStreamInterceptor automatically calls a second RecvMsg on the
213-
// underlying client stream to consume io.EOF and receive trailers.
214+
// underlying client stream to consume io.EOF and receive trailers. But this
215+
// should not be visible to user interceptors.
214216
stream, err := ss.Client.StreamingInputCall(ctx)
215217
if err != nil {
216218
t.Fatal("Error calling StreamingInputCall:", err)
@@ -221,17 +223,21 @@ func (s) TestDefaultStreamInterceptor(t *testing.T) {
221223
if _, err := stream.CloseAndRecv(); err != nil {
222224
t.Fatal("Error running CloseAndRecv:", err)
223225
}
224-
if iStream.recvMsgCount != 2 {
225-
t.Fatalf("StreamingInputCall RecvMsg was called %v times, want 2 times", iStream.recvMsgCount)
226+
if iStream.recvMsgCount != 1 {
227+
t.Fatalf("RecvMsg was called %v times on user interceptor stream, want 1 time", iStream.recvMsgCount)
226228
}
227229

228230
// Make a server-streaming RPC. Since StreamingOutputCall is not
229-
// client-streaming, defaultStreamInterceptor immediately invokes CloseSend
230-
// right after sending the request message to signal downstream interceptors.
231+
// client-streaming, the proto generated code invokes CloseSend after
232+
// sending the single message. defaultStreamInterceptor also invokes
233+
// CloseSend right after sending the request message (to handle cases where
234+
// the user is using the ClientStream API instead of the proto generated
235+
// code) to signal downstream interceptors, which in this case are xDS
236+
// filters. The user interceptor should not see this CloseSend call.
231237
if _, err := ss.Client.StreamingOutputCall(ctx, &testpb.StreamingOutputCallRequest{}); err != nil {
232238
t.Fatal("Error calling StreamingOutputCall:", err)
233239
}
234-
if !iStream.closeSend {
235-
t.Fatal("CloseSend not called after SendMsg on non-client-streaming RPC")
240+
if iStream.closeSendCount != 1 {
241+
t.Fatalf("CloseSend called %v times on user interceptor stream, want 1 times", iStream.closeSendCount)
236242
}
237243
}
Lines changed: 239 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,239 @@
1+
/*
2+
*
3+
* Copyright 2026 gRPC authors.
4+
*
5+
* Licensed under the Apache License, Version 2.0 (the "License");
6+
* you may not use this file except in compliance with the License.
7+
* You may obtain a copy of the License at
8+
*
9+
* http://www.apache.org/licenses/LICENSE-2.0
10+
*
11+
* Unless required by applicable law or agreed to in writing, software
12+
* distributed under the License is distributed on an "AS IS" BASIS,
13+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
* See the License for the specific language governing permissions and
15+
* limitations under the License.
16+
*
17+
*/
18+
19+
package xds_test
20+
21+
import (
22+
"context"
23+
"fmt"
24+
"io"
25+
"testing"
26+
27+
v3xdsxdstypepb "github.com/cncf/xds/go/xds/type/v3"
28+
v3clusterpb "github.com/envoyproxy/go-control-plane/envoy/config/cluster/v3"
29+
v3endpointpb "github.com/envoyproxy/go-control-plane/envoy/config/endpoint/v3"
30+
v3listenerpb "github.com/envoyproxy/go-control-plane/envoy/config/listener/v3"
31+
v3routepb "github.com/envoyproxy/go-control-plane/envoy/config/route/v3"
32+
v3httppb "github.com/envoyproxy/go-control-plane/envoy/extensions/filters/network/http_connection_manager/v3"
33+
"google.golang.org/grpc"
34+
"google.golang.org/grpc/credentials/insecure"
35+
"google.golang.org/grpc/internal/stubserver"
36+
"google.golang.org/grpc/internal/testutils"
37+
"google.golang.org/grpc/internal/testutils/xds/e2e"
38+
"google.golang.org/grpc/internal/testutils/xds/e2e/setup"
39+
"google.golang.org/grpc/internal/xds/httpfilter"
40+
41+
testgrpc "google.golang.org/grpc/interop/grpc_testing"
42+
testpb "google.golang.org/grpc/interop/grpc_testing"
43+
)
44+
45+
// Tests the interaction between the defaultStreamInterceptor and xDS filters.
46+
// It verifies that the SendMsg, RecvMsg, and CloseSend methods are called the
47+
// expected number of times for different RPC types (unary, client streaming,
48+
// server streaming, and bidi streaming) when an xDS filter is registered and
49+
// used in the call chain.
50+
func (s) TestDefaultStreamInterceptor_InteractionWithXDSFilters(t *testing.T) {
51+
// Register a custom xDS filter builder for the test.
52+
testFilterTypeURL := t.Name()
53+
filterBuilder := newTrackingHTTPFilterBuilder(testFilterTypeURL)
54+
httpfilter.Register(filterBuilder)
55+
defer httpfilter.UnregisterForTesting(testFilterTypeURL)
56+
57+
// Setup a test backend implementing both all four RPC types.
58+
testServer := &stubserver.StubServer{
59+
EmptyCallF: func(context.Context, *testpb.Empty) (*testpb.Empty, error) {
60+
return &testpb.Empty{}, nil
61+
},
62+
StreamingOutputCallF: func(_ *testpb.StreamingOutputCallRequest, stream testgrpc.TestService_StreamingOutputCallServer) error {
63+
return stream.Send(&testpb.StreamingOutputCallResponse{})
64+
},
65+
StreamingInputCallF: func(stream testgrpc.TestService_StreamingInputCallServer) error {
66+
for {
67+
_, err := stream.Recv()
68+
if err == io.EOF {
69+
break
70+
}
71+
if err != nil {
72+
return err
73+
}
74+
}
75+
return stream.SendAndClose(&testpb.StreamingInputCallResponse{})
76+
},
77+
FullDuplexCallF: func(stream testgrpc.TestService_FullDuplexCallServer) error {
78+
for {
79+
_, err := stream.Recv()
80+
if err == io.EOF {
81+
return nil
82+
}
83+
if err != nil {
84+
return err
85+
}
86+
if err := stream.Send(&testpb.StreamingOutputCallResponse{}); err != nil {
87+
return err
88+
}
89+
}
90+
},
91+
}
92+
if err := testServer.Start(nil); err != nil {
93+
t.Fatal("Error starting server:", err)
94+
}
95+
defer testServer.Stop()
96+
97+
// Start an xDS management server.
98+
mgmtServer, nodeID, _, xdsResolver := setup.ManagementServerAndResolver(t)
99+
100+
const serviceName = "my-service-xds"
101+
clusterSpec := &v3routepb.RouteAction_Cluster{Cluster: "cluster-A"}
102+
hcm := &v3httppb.HttpConnectionManager{
103+
RouteSpecifier: &v3httppb.HttpConnectionManager_RouteConfig{
104+
RouteConfig: &v3routepb.RouteConfiguration{
105+
Name: "route-" + serviceName,
106+
VirtualHosts: []*v3routepb.VirtualHost{{
107+
Domains: []string{serviceName},
108+
Routes: []*v3routepb.Route{{
109+
Match: &v3routepb.RouteMatch{PathSpecifier: &v3routepb.RouteMatch_Prefix{Prefix: ""}},
110+
Action: &v3routepb.Route_Route{Route: &v3routepb.RouteAction{
111+
ClusterSpecifier: clusterSpec,
112+
}},
113+
}},
114+
}},
115+
},
116+
},
117+
HttpFilters: []*v3httppb.HttpFilter{
118+
{
119+
Name: "tracking-filter",
120+
ConfigType: &v3httppb.HttpFilter_TypedConfig{
121+
TypedConfig: testutils.MarshalAny(t, &v3xdsxdstypepb.TypedStruct{
122+
TypeUrl: testFilterTypeURL,
123+
}),
124+
},
125+
},
126+
e2e.RouterHTTPFilter,
127+
},
128+
}
129+
resources := e2e.UpdateOptions{
130+
NodeID: nodeID,
131+
Listeners: []*v3listenerpb.Listener{{Name: serviceName, ApiListener: &v3listenerpb.ApiListener{ApiListener: testutils.MarshalAny(t, hcm)}}},
132+
Clusters: []*v3clusterpb.Cluster{e2e.DefaultCluster("cluster-A", "cluster-A", e2e.SecurityLevelNone)},
133+
Endpoints: []*v3endpointpb.ClusterLoadAssignment{e2e.DefaultEndpoint("cluster-A", "localhost", []uint32{testutils.ParsePort(t, testServer.Address)})},
134+
}
135+
136+
ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
137+
defer cancel()
138+
if err := mgmtServer.Update(ctx, resources); err != nil {
139+
t.Fatal(err)
140+
}
141+
142+
cc, err := grpc.NewClient(fmt.Sprintf("xds:///%s", serviceName), grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithResolvers(xdsResolver))
143+
if err != nil {
144+
t.Fatalf("Failed to create a gRPC client: %v", err)
145+
}
146+
defer cc.Close()
147+
client := testgrpc.NewTestServiceClient(cc)
148+
149+
tests := []struct {
150+
name string
151+
run func(ctx context.Context, client testgrpc.TestServiceClient)
152+
wantSendMsg int32
153+
wantRecvMsg int32
154+
}{
155+
{
156+
name: "unary",
157+
run: func(ctx context.Context, client testgrpc.TestServiceClient) {
158+
if _, err := client.EmptyCall(ctx, &testpb.Empty{}); err != nil {
159+
t.Fatalf("EmptyCall() failed: %v", err)
160+
}
161+
},
162+
wantSendMsg: 1, // 1 request
163+
wantRecvMsg: 2, // 1 response + 1 trailers
164+
},
165+
{
166+
name: "client_streaming",
167+
run: func(ctx context.Context, client testgrpc.TestServiceClient) {
168+
stream, err := client.StreamingInputCall(ctx)
169+
if err != nil {
170+
t.Fatalf("StreamingInputCall() failed: %v", err)
171+
}
172+
for i := 0; i < 2; i++ {
173+
if err := stream.Send(&testpb.StreamingInputCallRequest{}); err != nil {
174+
t.Fatalf("Send() failed: %v", err)
175+
}
176+
}
177+
if _, err := stream.CloseAndRecv(); err != nil {
178+
t.Fatalf("CloseAndRecv() failed: %v", err)
179+
}
180+
},
181+
wantSendMsg: 2, // 2 requests
182+
wantRecvMsg: 2, // 1 response + 1 trailers
183+
},
184+
{
185+
name: "server_streaming",
186+
run: func(ctx context.Context, client testgrpc.TestServiceClient) {
187+
stream, err := client.StreamingOutputCall(ctx, &testpb.StreamingOutputCallRequest{})
188+
if err != nil {
189+
t.Fatalf("StreamingOutputCall() failed: %v", err)
190+
}
191+
if _, err := stream.Recv(); err != nil {
192+
t.Fatalf("Recv() failed: %v", err)
193+
}
194+
},
195+
wantSendMsg: 1, // 1 request
196+
wantRecvMsg: 1, // 1 response (trailers not yet consumed since stream not read to EOF)
197+
},
198+
{
199+
name: "bidi_streaming",
200+
run: func(ctx context.Context, client testgrpc.TestServiceClient) {
201+
stream, err := client.FullDuplexCall(ctx)
202+
if err != nil {
203+
t.Fatalf("FullDuplexCall() failed: %v", err)
204+
}
205+
if err := stream.Send(&testpb.StreamingOutputCallRequest{}); err != nil {
206+
t.Fatalf("Send() failed: %v", err)
207+
}
208+
if _, err := stream.Recv(); err != nil {
209+
t.Fatalf("Recv() failed: %v", err)
210+
}
211+
if err := stream.CloseSend(); err != nil {
212+
t.Fatalf("CloseSend() failed: %v", err)
213+
}
214+
},
215+
wantSendMsg: 1, // 1 request
216+
wantRecvMsg: 1, // 1 response
217+
},
218+
}
219+
220+
for _, tc := range tests {
221+
t.Run(tc.name, func(t *testing.T) {
222+
filterBuilder.sendMsgCount.Store(0)
223+
filterBuilder.recvMsgCount.Store(0)
224+
filterBuilder.closeSendCount.Store(0)
225+
226+
tc.run(ctx, client)
227+
228+
if got := filterBuilder.sendMsgCount.Load(); got != tc.wantSendMsg {
229+
t.Fatalf("SendMsg() count = %d, want %d", got, tc.wantSendMsg)
230+
}
231+
if got := filterBuilder.recvMsgCount.Load(); got != tc.wantRecvMsg {
232+
t.Fatalf("RecvMsg() count = %d, want %d", got, tc.wantRecvMsg)
233+
}
234+
if got := filterBuilder.closeSendCount.Load(); got != 1 {
235+
t.Fatalf("CloseSend() count = %d, want 1", got)
236+
}
237+
})
238+
}
239+
}

0 commit comments

Comments
 (0)