rafthttp: use http.Request.WithContext instead of Cancel #7255

sinsharat · 2017-01-31T12:44:04Z

sinsharat · 2017-01-31T16:08:06Z

@xiang90 @heyitsanthony PTAL. Don't think the jenkins-proxy-ci is failing due to this change. Have already tested e2e, rafthttptest and integration in my local machine.
Proof attched:

Thanks!

xiang90 · 2017-01-31T17:15:09Z

rafthttp/pipeline.go

 	"github.com/coreos/etcd/pkg/pbutil"
 	"github.com/coreos/etcd/pkg/types"
 	"github.com/coreos/etcd/raft"
 	"github.com/coreos/etcd/raft/raftpb"
+
+	"golang.org/x/net/context"


use https://golang.org/pkg/context/ instead?

xiang90 · 2017-01-31T17:17:19Z

rafthttp/pipeline.go

-		// always be stopped. So we use reportCriticalError to report it to errorc.
-		if err == errMemberRemoved {
-			reportCriticalError(err, p.errorc)
+	go func() {


why put this into a go routine? what's wrong with the previous code?

@xiang90 i have coded a select statement after this so that after cancel the program exits. Without this the test case was failing on using request.WithCancel. Let me try again.

@xiang90 i tested it with old code, it will not work. The test case fails. So this code is required for it to work properly in this case.

Why the old work works? But now it does not? I think we should figure it out. The only change how to set the cancel(), right? It should not affect the code after it I assume.

@xiang90 I had already checked the code earlier of go package. With old code it checks for cancel. i mean its cancel func(depricated) But with the new implementation it needs the check for ctx.Done. The above code ensures the proper closure of request before exiting. Without this code, it doesn't.

I do not think I understand what you said.

Let me clarify it.

now, we:

create context with cancel at line 122

pass in the created context to req at line 123

Now if the cancel fun get called, the request should be canceled.

create a go routine to listen on stop. if server is stopped, we can cancel.

So if server is stopped, go routine should be triggered on stop channel, and call cancel.

After it calls cancel, the http request should be canceled.

From your test, it fails to cancel the http request. There is something goes wrong.

You "fix" that by making the http request nonblocking. That is not what we want. We want the request to be canceled once the server is stopped. Basically we need to make sure the go routine created at 134 exits.

@xiang90 ok will check it out.

@xiang90 i went through the code and i found that the issue is with the test case. Earlier the request cancel was being handled by us manually, hence the roundTripperBlocker used for test case is checking for req.Cancel which used to come earlier from our manual implementation. Now that is not the case and since the roundTripperBlocker has no implementation for ctx.Done hence it was failing.
Will correct the same and update the PR. Thanks!

xiang90 · 2017-01-31T17:17:30Z

rafthttp/snapshot_sender.go

@@ -26,6 +26,8 @@ import (
 	"github.com/coreos/etcd/pkg/types"
 	"github.com/coreos/etcd/raft"
 	"github.com/coreos/etcd/snap"
+
+	"golang.org/x/net/context"


use https://golang.org/pkg/context/

xiang90 · 2017-01-31T17:17:44Z

rafthttp/stream.go

@@ -410,6 +412,7 @@ func (cr *streamReader) stop() {
 }

 func (cr *streamReader) dial(t streamType) (io.ReadCloser, error) {
+


remove this extra empty line

xiang90 · 2017-01-31T17:20:16Z

Ignore proxy-ci for now.

heyitsanthony · 2017-01-31T17:27:26Z

@sinsharat looks like a new failure. I tried to reproduce with go test -run=RejectInsecure -v -tags cluster_proxy from your brach but it seems OK...

sinsharat · 2017-01-31T17:57:03Z

@xiang90 @heyitsanthony all changes done. PTAL.
Thanks!

sinsharat · 2017-02-01T12:38:37Z

@xiang90 @heyitsanthony All fixed. PTAL.
Thanks!

xiang90 · 2017-02-01T17:43:07Z

rafthttp/snapshot_sender.go

-	cancel := httputil.RequestCanceler(req)
+	ctx, cancel := context.WithCancel(context.Background())
+	req = req.WithContext(ctx)
+	defer cancel()


revert this change ( defer cancel() )? or change L128 of rafthttp/pipeline.go to make them consistent?

@xiang90 i had to do this change because the govet in CI was giving a possible context leak error with old cancel placed there since it expects cancel to be called mandatorily before exiting. What do you want me to do for this?

can you also fix other places? like L128 of rafthttp/pipeline.go

@xiang90 for that we will need to change the design a bit since we cannot use defer directly there. For this reason govmt is not giving the error there. Using defer will not cause the code to work properly. So we will need to move the go routine code to i.e. L124 to L128 outside go routine and the code from L132 to L156 inside the go routine. Is that fine with you? Also this will make the code very similary to snapshot sender.go

ok. we can deal with that later.

xiang90 · 2017-02-01T17:44:09Z

looks great. there are a few minor issues. we need to continue to clean up other pkgs if there is any. Thanks @sinsharat!

sinsharat · 2017-02-01T18:23:38Z

@xiang90 do you want other packages to be corrected in the same PR?

xiang90 · 2017-02-01T20:01:42Z

@sinsharat No. We can do them in other prs :P

sinsharat · 2017-02-01T20:21:32Z

@xiang90 Ok. Let me know if anything else required to be done here. Will start with other packages once this gets merged.
Thank you!

xiang90 · 2017-02-01T20:54:08Z

LGTM

xiang90 · 2017-02-01T20:54:36Z

@sinsharat Actually can you squash two commits into one?

sinsharat · 2017-02-01T21:02:11Z

@xiang90 its squashed into one.
Thanks!

xiang90 · 2017-02-01T23:49:56Z

Thank you!

fanminshi · 2017-02-01T23:53:08Z

rafthttp/pipeline.go

@@ -16,13 +16,13 @@ package rafthttp

 import (
 	"bytes"
+	"context"


should we import "golang.org/x/net/context" instead of context? @xiang90 @heyitsanthony .

@fanminshi we will use context in 3.2 release. we do not need the compatibility anymore.

got it. Also have we refractor our code to use context yet?

xiang90 reviewed Jan 31, 2017

View reviewed changes

xiang90 reviewed Feb 1, 2017

View reviewed changes

xiang90 closed this Feb 1, 2017

xiang90 reopened this Feb 1, 2017

rafthttp: use http.Request.WithContext instead of Cancel

2656b59

xiang90 merged commit 42e7d4d into etcd-io:master Feb 1, 2017

fanminshi reviewed Feb 1, 2017

View reviewed changes

sinsharat deleted the use_requestWithContext_for_cancel branch February 2, 2017 12:59

xiang90 mentioned this pull request Feb 2, 2017

use http.Request.WithContext instead of Cancel #6953

Closed

		@@ -410,6 +412,7 @@ func (cr *streamReader) stop() {
		}

		func (cr *streamReader) dial(t streamType) (io.ReadCloser, error) {

rafthttp: use http.Request.WithContext instead of Cancel #7255

rafthttp: use http.Request.WithContext instead of Cancel #7255

Conversation

sinsharat commented Jan 31, 2017

sinsharat commented Jan 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sinsharat Jan 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sinsharat Jan 31, 2017 • edited Loading

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

Choose a reason for hiding this comment

xiang90 commented Jan 31, 2017

heyitsanthony commented Jan 31, 2017

sinsharat commented Jan 31, 2017

sinsharat commented Feb 1, 2017

Choose a reason for hiding this comment

sinsharat Feb 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sinsharat Feb 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiang90 commented Feb 1, 2017

sinsharat commented Feb 1, 2017

xiang90 commented Feb 1, 2017

sinsharat commented Feb 1, 2017 • edited Loading

xiang90 commented Feb 1, 2017

xiang90 commented Feb 1, 2017

sinsharat commented Feb 1, 2017

xiang90 commented Feb 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sinsharat commented Jan 31, 2017 •

edited

Loading

sinsharat Jan 31, 2017 •

edited

Loading

sinsharat Jan 31, 2017 •

edited

Loading

sinsharat Feb 1, 2017 •

edited

Loading

sinsharat Feb 1, 2017 •

edited

Loading

sinsharat commented Feb 1, 2017 •

edited

Loading