Skip to content

rpc: fix passing of ServerStreams to RangeFeed local RPCs#82621

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
andreimatei:rpc.fix-stream
Jun 8, 2022
Merged

rpc: fix passing of ServerStreams to RangeFeed local RPCs#82621
craig[bot] merged 1 commit intocockroachdb:masterfrom
andreimatei:rpc.fix-stream

Conversation

@andreimatei
Copy link
Copy Markdown
Contributor

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 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 #82603
Fixes #82579
Fixes #82581
Fixes #82585
Fixes #82577

Release note: None

@andreimatei andreimatei requested a review from a team as a code owner June 8, 2022 18:26
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

Copy link
Copy Markdown
Contributor

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

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

:lgtm: Thanks for the continued cleanups and improvements here!

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: 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.

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
@andreimatei
Copy link
Copy Markdown
Contributor Author

TFTR!

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jun 8, 2022

Build succeeded:

@craig craig bot merged commit 7d365bc into cockroachdb:master Jun 8, 2022
@andreimatei andreimatei deleted the rpc.fix-stream branch June 9, 2022 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants