transport: ensure client always closes streams #2354

siggy · 2018-10-06T00:13:28Z

When http2Client receives END_STREAM from the server, it calls
closeStream, but does not send END_STREAM back to the server. This
leaves the stream in a half-closed state.

This change ensures that END_STREAM is sent back to the server,
putting the stream in a fully closed state. This motivated modifying the
streamState type to more closely model the state transitions defined
in https://http2.github.io/http2-spec/#StreamStates. Specifically:

active -> [recv ES] -> readDone
active -> [send ES] -> writeDone
readDone -> [send ES] -> done
writeDone -> [recv ES] -> done

Reproduction

PCAP files, one before the change, one after:
grpc-end-stream-pcap.zip

Existing behavior (without END_STREAM)

Get test repo

go get github.com/buoyantio/strest-grpc
cd $GOPATH/src/github.com/buoyantio/strest-grpc

Start server

go run main.go ref-server
INFO[0000] starting gRPC server on :11111

Run client

(rpc error is expected here, just needed a way to reproduce a server sending an END_STREAM)

go run main.go ref-client
INFO[0000] connecting to localhost:11111
ERRO[0000] stream.Recv() returned unexpected error: rpc error: code = Unknown desc = invalid ResponseSpec, Count cannot be negative

New behavior (with END_STREAM)

Switch to modified branch (includes this PR's change in vendor)

git checkout siggy/grpc-eos-test

Start server

go run main.go ref-server
INFO[0000] starting gRPC server on :11111

Run client

go run main.go ref-client
INFO[0000] connecting to localhost:11111
ERRO[0000] stream.Recv() returned unexpected error: rpc error: code = Unknown desc = invalid ResponseSpec, Count cannot be negative

When `http2Client` receives `END_STREAM` from the server, it calls `closeStream`, but does not send `END_STREAM` back to the server. This leaves the stream in a half-closed state. This change ensures that `END_STREAM` is sent back to the server, putting the stream in a fully closed state. This motivated modifying the `streamState` type to more closely model the state transitions defined in https://http2.github.io/http2-spec/#StreamStates. Specifically: - active -> [recv ES] -> readDone - active -> [send ES] -> writeDone - readDone -> [send ES] -> done - writeDone -> [recv ES] -> done

siggy · 2018-10-06T00:27:00Z

internal/transport/http2_client.go

@@ -680,8 +680,13 @@ func (t *http2Client) CloseStream(s *Stream, err error) {
 }

 func (t *http2Client) closeStream(s *Stream, err error, rst bool, rstCode http2.ErrCode, st *status.Status, mdata map[string][]string, eosReceived bool) {
+	if !s.isStreamWriteDone() {
+		// send END_STREAM
+		t.Write(s, nil, nil, &Options{Last: true})


Just calling out this is where the behavior change is. I want to confirm that it's safe to attempt a http2Client.Write() here, particularly because our call stack originates with a read from the server (where it receives an END_STREAM):

http2Client.Write()

http2Client.closeStream()

http2Client.operateHeaders()

http2Client.reader()

From debug.Stack():

github.com/buoyantio/strest-grpc/vendor/google.golang.org/grpc/internal/transport.(*http2Client).closeStream(0xc00009ed00, 0xc0001d6000, 0x1765980, 0xc000090050, 0x1e8000, 0xc0001ea010, 0xc0001e8120, 0x1) /Users/sig/code/go/src/github.com/buoyantio/strest-grpc/vendor/google.golang.org/grpc/internal/transport/http2_client.go:669 +0x302 github.com/buoyantio/strest-grpc/vendor/google.golang.org/grpc/internal/transport.(*http2Client).operateHeaders(0xc00009ed00, 0xc0001e8090) /Users/sig/code/go/src/github.com/buoyantio/strest-grpc/vendor/google.golang.org/grpc/internal/transport/http2_client.go:1156 +0x2f4 github.com/buoyantio/strest-grpc/vendor/google.golang.org/grpc/internal/transport.(*http2Client).reader(0xc00009ed00) /Users/sig/code/go/src/github.com/buoyantio/strest-grpc/vendor/google.golang.org/grpc/internal/transport/http2_client.go:1213 +0x70f created by github.com/buoyantio/strest-grpc/vendor/google.golang.org/grpc/internal/transport.newHTTP2Client /Users/sig/code/go/src/github.com/buoyantio/strest-grpc/vendor/google.golang.org/grpc/internal/transport/http2_client.go:263 +0xaf8

This should work, but let's do a RST_STREAM instead.

The ideal process of a normal RPC is, the client sends end-of-stream when it's done sending, and the server finishes the RPC with trailer (and also is end-of-stream).
So when the client receives an end-of-stream before it sends end-of-stream, it is unexpected. So RST_STREAM sounds better here.

closeStream is already sending RST_STREAM to terminate the stream when necessary. The parameters rst and rstCode are for that. So if you set rst to true, things should happen automatically. We can use http2.ErrCodeNo for the error code.

menghanl

Thanks for the fix!

I have some comments on END_STREAM vs RST_STREAM. Let me know what you think. Thanks!

menghanl · 2018-10-08T22:44:55Z

internal/transport/transport.go

@@ -195,7 +195,9 @@ type Stream struct {
 	// On the server-side, headerSent is atomically set to 1 when the headers are sent out.
 	headerSent uint32

-	state streamState
+	// stateMu protects stream state transitions
+	stateMu sync.Mutex


We tried to avoid this mutex here for performance reasons, and that's why we do those atomic actions.

The state transition in gRPC is limited comparing with the http2 spec:

the client will never be in read-done mode because that means the end of the stream, so the transition can only be active -> [writeDone] -> done

the server will never be in write-done mode, and it can only be active -> [readDone] -> done

However, because the Stream struct is shared by the client and the server, so the state is made a atomic, and the logic is handled in the client/server specific code. We have plans to split Stream for client and server, and will be able to better solve this problem.

I would suggest let's revert this back to the previous state if you are OK with it.

menghanl · 2018-10-08T23:38:48Z

internal/transport/http2_client.go

@@ -680,8 +680,13 @@ func (t *http2Client) CloseStream(s *Stream, err error) {
 }

 func (t *http2Client) closeStream(s *Stream, err error, rst bool, rstCode http2.ErrCode, st *status.Status, mdata map[string][]string, eosReceived bool) {
+	if !s.isStreamWriteDone() {
+		// send END_STREAM
+		t.Write(s, nil, nil, &Options{Last: true})


This should work, but let's do a RST_STREAM instead.

The ideal process of a normal RPC is, the client sends end-of-stream when it's done sending, and the server finishes the RPC with trailer (and also is end-of-stream).
So when the client receives an end-of-stream before it sends end-of-stream, it is unexpected. So RST_STREAM sounds better here.

closeStream is already sending RST_STREAM to terminate the stream when necessary. The parameters rst and rstCode are for that. So if you set rst to true, things should happen automatically. We can use http2.ErrCodeNo for the error code.

based on review feedback Signed-off-by: Andrew Seigner <[email protected]>

changes based on grpc/grpc-go#2354 Signed-off-by: Andrew Seigner <[email protected]>

siggy · 2018-10-09T01:49:12Z

Thanks for the thorough review @menghanl ! I've made your suggested changes, much simpler.

Confirmed RST_STREAM is now sent from the client:

grpc-rst-stream.pcapng.zip

`maxConcurrentStreamsPerConnection`'s default value was unlimited, meaning Linkerd could run out of memory if a client created enough streams, particularly if the client is leaking streams but not properly closing them. Change default `maxConcurrentStreamsPerConnection` value to 1000, disallow unlimited via a Linkerd config file. Related to #2132 and grpc/grpc-go#2354 Signed-off-by: Andrew Seigner <[email protected]>

menghanl

Thanks for the fix! LGTM.

siggy commented Oct 6, 2018

View reviewed changes

menghanl reviewed Oct 8, 2018

View reviewed changes

revert previous, END_STREAM commit, use RST_STREAM

a82efe6

based on review feedback Signed-off-by: Andrew Seigner <[email protected]>

siggy added a commit to BuoyantIO/strest-grpc that referenced this pull request Oct 9, 2018

More testing

f66d7fb

changes based on grpc/grpc-go#2354 Signed-off-by: Andrew Seigner <[email protected]>

siggy changed the title ~~internal: ensure http2Client sends END_STREAM~~ internal: ensure http2Client sends RST_STREAM Oct 9, 2018

siggy mentioned this pull request Oct 10, 2018

set maxConcurrentStreamsPerConnection default 1000 linkerd/linkerd#2146

Merged

menghanl approved these changes Oct 15, 2018

View reviewed changes

menghanl changed the title ~~internal: ensure http2Client sends RST_STREAM~~ transport: ensure http2Client sends RST_STREAM Oct 15, 2018

menghanl merged commit 1da8e51 into grpc:master Oct 15, 2018

menghanl added the Type: Bug label Oct 15, 2018

menghanl added this to the 1.16 Release milestone Oct 15, 2018

menghanl changed the title ~~transport: ensure http2Client sends RST_STREAM~~ transport: ensure client always closes streams Oct 23, 2018

lock bot locked as resolved and limited conversation to collaborators Apr 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

transport: ensure client always closes streams #2354

transport: ensure client always closes streams #2354

siggy commented Oct 6, 2018 •

edited

Loading

siggy Oct 6, 2018 •

edited

Loading

menghanl Oct 8, 2018

menghanl left a comment

menghanl Oct 8, 2018

menghanl Oct 8, 2018

siggy commented Oct 9, 2018

menghanl left a comment

transport: ensure client always closes streams #2354

transport: ensure client always closes streams #2354

Conversation

siggy commented Oct 6, 2018 • edited Loading

Reproduction

Existing behavior (without END_STREAM)

Get test repo

Start server

Run client

New behavior (with END_STREAM)

Switch to modified branch (includes this PR's change in vendor)

Start server

Run client

siggy Oct 6, 2018 • edited Loading

Choose a reason for hiding this comment

menghanl Oct 8, 2018

Choose a reason for hiding this comment

menghanl left a comment

Choose a reason for hiding this comment

menghanl Oct 8, 2018

Choose a reason for hiding this comment

menghanl Oct 8, 2018

Choose a reason for hiding this comment

siggy commented Oct 9, 2018

menghanl left a comment

Choose a reason for hiding this comment

siggy commented Oct 6, 2018 •

edited

Loading

siggy Oct 6, 2018 •

edited

Loading