transport: refactor of error/cancellation paths #1533

dfawley · 2017-09-21T17:44:13Z

The transport is now responsible for closing its own connection when an error
occurs or when the context given to it in NewClientTransport() is canceled.
Remove client/server shutdown channels -- add cancel function to allow
self-cancellation.
Plumb the clientConn's context into the client transport to allow for the
transport to be canceled even after it has been removed from the ac (due to
graceful close) when the ClientConn is closed.

This is pre-work for fixing #1532 and implementing transparent retries.

menghanl · 2017-09-25T21:27:50Z

transport/http2_client.go

 	scheme := "http"
-	conn, err := dial(ctx, opts.Dialer, addr.Addr)
+	ctx, cancel := context.WithCancel(ctx)
+	connectCtx, connectCancel := context.WithTimeout(ctx, timeout)


Move these two cancel functions to a defer

defer func() { if err != nil { cancel() // Don't call connectCancel in success path due to a race in Go 1.6: // https://github.com/golang/go/issues/15078. connectCancel() } }

Done. The reason the conn is passed in the defer below, BTW, is because if ClientHandshake encounters an error, conn would probably be nil. I tried to clean this up, but it's a slippery slope and the diffs start looking much bigger. I'll wait until later and only do what you asked for now.

menghanl · 2017-09-26T23:40:01Z

transport/http2_client.go

@@ -601,7 +608,7 @@ func (t *http2Client) Close() (err error) {
 	}


This if will always be true. The only other state is unreachable set by notifyError, and that code was deleted.
Actually, is errorChan === t.ctx.Done()?

Removed the if, and unreachable entirely.

Yes, errorChan is essentially t.ctx.Done(), with only subtly different semantics that were irrelevant. Removed. Good catch.

menghanl · 2017-09-29T18:31:20Z

Conflicts...

Also, should we label this PR as an API change? Even though it's an internal API change?

dfawley · 2017-09-29T18:51:33Z

Conflicts...

Resolved.

Also, should we label this PR as an API change? Even though it's an internal API change?

I don't think so. I think the labels on PRs should be applied so that end users can understand the release notes. If we put non-user-impacting API changes in with ones that do impact users, that dilutes the significance of the section.

menghanl

LGTM 💪

- The transport is now responsible for closing its own connection when an error occurs or when the context given to it in NewClientTransport() is canceled. - Remove client/server shutdown channels -- add cancel function to allow self-cancellation. - Plumb the clientConn's context into the client transport to allow for the transport to be canceled even after it has been removed from the ac (due to graceful close) when the ClientConn is closed.

tamird · 2017-10-12T20:35:21Z

vet.sh

@@ -65,7 +65,7 @@ git ls-files "*.go" | xargs sed -i 's:"golang.org/x/net/context":"context":'
 set +o pipefail
 # TODO: Stop filtering pb.go files once golang/protobuf#214 is fixed.
 # TODO: Remove clientconn exception once go1.6 support is removed.


looks like you addressed this TODO.

The comment was removed when Go1.6 support was removed.

tamird · 2017-10-12T20:36:20Z

transport/transport.go

-// If it receives from proceed, it returns the received integer, nil.
-func wait(ctx context.Context, done, goAway, closing <-chan struct{}, proceed <-chan int) (int, error) {
+// wait blocks until it can receive from one of the provided contexts or channels
+func wait(ctx, tctx context.Context, done, goAway <-chan struct{}, proceed <-chan int) (int, error) {


what is tctx? the name leaves something to be desired and the doc comment doesn't explain it.

t=transport. I'll add a comment. I wasn't a big fan of this function in the first place, but I decided to keep it (for now anyway).

tamird · 2017-10-12T20:38:14Z

transport/http2_server.go

 	default:
-		errorf("transport: http2Server.controller got unexpected item type %v\n", i)
+		err := status.Errorf(codes.Internal, "transport: http2Server.controller got unexpected item type %t\n", i)


I believe it's conventional to omit trailing newlines in error strings.

This was fixed by #1553.

dfawley

Thanks for taking a look.

dfawley · 2017-10-12T21:08:00Z

transport/http2_server.go

 	default:
-		errorf("transport: http2Server.controller got unexpected item type %v\n", i)
+		err := status.Errorf(codes.Internal, "transport: http2Server.controller got unexpected item type %t\n", i)


This was fixed by #1553.

dfawley · 2017-10-12T21:09:22Z

transport/transport.go

-// If it receives from proceed, it returns the received integer, nil.
-func wait(ctx context.Context, done, goAway, closing <-chan struct{}, proceed <-chan int) (int, error) {
+// wait blocks until it can receive from one of the provided contexts or channels
+func wait(ctx, tctx context.Context, done, goAway <-chan struct{}, proceed <-chan int) (int, error) {


t=transport. I'll add a comment. I wasn't a big fan of this function in the first place, but I decided to keep it (for now anyway).

dfawley · 2017-10-12T21:12:38Z

vet.sh

@@ -65,7 +65,7 @@ git ls-files "*.go" | xargs sed -i 's:"golang.org/x/net/context":"context":'
 set +o pipefail
 # TODO: Stop filtering pb.go files once golang/protobuf#214 is fixed.
 # TODO: Remove clientconn exception once go1.6 support is removed.


The comment was removed when Go1.6 support was removed.

dfawley assigned menghanl Sep 21, 2017

dfawley requested a review from menghanl September 21, 2017 17:44

menghanl reviewed Sep 26, 2017

View reviewed changes

menghanl added this to the 1.7 Release milestone Sep 29, 2017

menghanl assigned menghanl and dfawley and unassigned menghanl Sep 29, 2017

dfawley force-pushed the better_context branch from a70c753 to d0d2db3 Compare September 29, 2017 18:51

dfawley assigned menghanl and unassigned dfawley Sep 29, 2017

menghanl approved these changes Sep 29, 2017

View reviewed changes

dfawley added 4 commits October 2, 2017 09:48

defer cancel sooner; remove unreachable state

6b02bdf

remove errorChan from client

a683f6f

gofmt

6de94a3

dfawley force-pushed the better_context branch from d0d2db3 to 6de94a3 Compare October 2, 2017 18:47

dfawley merged commit 5a82377 into grpc:master Oct 2, 2017

dfawley deleted the better_context branch October 2, 2017 18:56

menghanl added the Type: Internal Cleanup Refactors, etc label Oct 4, 2017

dfawley mentioned this pull request Oct 12, 2017

timeouts, addresses, and networks are a mess #758

Closed

tamird reviewed Oct 12, 2017

View reviewed changes

dfawley commented Oct 12, 2017

View reviewed changes

dfawley mentioned this pull request Oct 12, 2017

transport: minor cleanups (comment and error text) #1576

Merged

lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

transport: refactor of error/cancellation paths #1533

transport: refactor of error/cancellation paths #1533

dfawley commented Sep 21, 2017

menghanl Sep 25, 2017

dfawley Sep 27, 2017

menghanl Sep 26, 2017 •

edited

Loading

dfawley Sep 28, 2017

menghanl commented Sep 29, 2017

dfawley commented Sep 29, 2017

menghanl left a comment

tamird Oct 12, 2017

dfawley Oct 12, 2017

tamird Oct 12, 2017

dfawley Oct 12, 2017

tamird Oct 12, 2017

dfawley Oct 12, 2017

dfawley left a comment

dfawley Oct 12, 2017

dfawley Oct 12, 2017

dfawley Oct 12, 2017

		@@ -601,7 +608,7 @@ func (t *http2Client) Close() (err error) {
		}

transport: refactor of error/cancellation paths #1533

transport: refactor of error/cancellation paths #1533

Conversation

dfawley commented Sep 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

menghanl Sep 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

menghanl commented Sep 29, 2017

dfawley commented Sep 29, 2017

menghanl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfawley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

menghanl Sep 26, 2017 •

edited

Loading