rpc: fix passing of ServerStreams to RangeFeed local RPCs#82621
Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom Jun 8, 2022
Merged
rpc: fix passing of ServerStreams to RangeFeed local RPCs#82621craig[bot] merged 1 commit intocockroachdb:masterfrom
craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
Member
erikgrinaker
approved these changes
Jun 8, 2022
Contributor
erikgrinaker
left a comment
There was a problem hiding this comment.
Thanks for the continued cleanups and improvements here!
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @andreimatei)
-- commits line 35 at r1:
These assertions are proving pretty useful at uncovering subtle issues like these.
pkg/rpc/context.go line 918 at r1 (raw file):
// // The flow of data through the pipe, from producer to consumer: // RPC handler (i.e. Node.RangeFeed) ->
Thanks for the diagram, this is helpful.
6d49321 to
8060a14
Compare
We recently started running grpc interceptors on local RPCs (in cockroachdb#76306). There's a bug in there causing server-side streaming interceptors to not work as intended. A server-side streaming interceptor is supposed to wrap a streaming RPC handler and it has the option of overriding the ServerStream provided to the handler with a custom one. An RPC handler uses the ServerStream to communicate with the caller; custom implementations of ServerStream generally wrap the gRPC implementation, adding more transparent functionality (for example, they can override the Context that the handler runs in). The bug was causing the custom ServerStream provided by the interceptors to not actually be passed to the RPC handler for local RPCs (i.e. for streaming RPCs going through the internalClientAdapter - and there's only one such RPC: RangeFeed). Instead, the handler always was getting the vanilla gRPC ServerStream. The bug was in the following lines in internalClientAdapter.RangeFeed(): rfAdapter := rangeFeedServerAdapter{rangeFeedPipe: streamPipe} handler := func(srv interface{}, stream grpc.ServerStream) error { return a.server.RangeFeed(args, rfAdapter) } We're passing `rfAdapter` to a.server.RangeFeed(), when we should be passing `stream`. Making that change needs a bit of massaging, though. I've also cleaned up the code surrounding the streaming interceptors and tried to explain it better, as it was hard to follow. I've also added a test. The code still remains pretty convoluted, but I believe that that's inherently necessary given the gRPC interfaces we're working with. The bug was manifesting itself as span use-after-finish crashes in tests for the RangeFeed RPC. Because the RangeFeed handler was not using the ServerStream provided by the tracing server-side streaming interceptor, the handler was running in the client's Context, with the client-side tracing span. The client's (DistSender's) span is sometimes finished while the server-side RPC is still running, resulting in the use-after-finish. Fixes cockroachdb#82571 Fixes cockroachdb#82577 Fixes cockroachdb#82578 Fixes cockroachdb#82579 Fixes cockroachdb#82580 Fixes cockroachdb#82581 Fixes cockroachdb#82583 Fixes cockroachdb#82585 Fixes cockroachdb#82603 Release note: None
8060a14 to
80f9d53
Compare
Contributor
Author
|
TFTR! bors r+ |
Contributor
|
Build succeeded: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We recently started running grpc interceptors on local RPCs (in #76306).
There's a bug in there causing server-side streaming interceptors to not
work as intended. A server-side streaming interceptor is supposed to
wrap a streaming RPC handler and it has the option of overriding the
ServerStream provided to the handler with a custom one. An RPC handler
uses the ServerStream to communicate with the caller; custom
implementations of ServerStream generally wrap the gRPC implementation,
adding more transparent functionality (for example, they can override
the Context that the handler runs in).
The bug was causing the custom ServerStream provided by the interceptors
to not actually be passed to the RPC handler for local RPCs (i.e. for
streaming RPCs going through the internalClientAdapter - and there's
only one such RPC: RangeFeed). Instead, the handler always was getting
the vanilla gRPC ServerStream.
The bug was in the following lines in internalClientAdapter.RangeFeed():
rfAdapter := rangeFeedServerAdapter{rangeFeedPipe: streamPipe}
handler := func(srv interface{}, stream grpc.ServerStream) error {
return a.server.RangeFeed(args, rfAdapter)
}
We're passing
rfAdapterto a.server.RangeFeed(), when we should bepassing
stream. Making that change needs a bit of massaging, though.I've also cleaned up the code surrounding the streaming interceptors and
tried to explain it better, as it was hard to follow. I've also added a
test. The code still remains pretty convoluted, but I believe that
that's inherently necessary given the gRPC interfaces we're working
with.
The bug was manifesting itself as span use-after-finish crashes in
tests for the RangeFeed RPC. Because the RangeFeed handler was not using
the ServerStream provided by the tracing server-side streaming
interceptor, the handler was running in the client's Context, with the
client-side tracing span. The client's (DistSender's) span is sometimes
finished while the server-side RPC is still running, resulting in the
use-after-finish.
Fixes #82603
Fixes #82579
Fixes #82581
Fixes #82585
Fixes #82577
Release note: None