net/http/httputil: make Switching Protocol requests (e.g. Websockets) cancelable #38021

piec · 2020-03-23T12:13:25Z

Ensures that a canceled client request for Switching Protocols
(e.g. h2c, Websockets) will cause the underlying connection to
be terminated.

Adds a goroutine in handleUpgradeResponse in order to select on
the incoming client request's context and appropriately cancel it.

Fixes #35559

gopherbot · 2020-03-23T12:15:26Z

This PR (HEAD: f69f4f9) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/224897 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2020-03-23T12:16:47Z

Message from Gobot Gobot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
Within the next week or so, a maintainer will review your change and provide
feedback. See https://golang.org/doc/contribute.html#review for more info and
tips to get your patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11, it means that this CL will be reviewed as part of the next development
cycle. See https://golang.org/s/release for more details.

Please don’t reply on this GitHub thread. Visit golang.org/cl/224897.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-03-23T22:01:24Z

Message from Emmanuel Odeke:

Patch Set 1:

(24 comments)

Thank you for this change Pierre and welcome to the Go project!

I have added some suggestions, please take a look. Also given that
you made a Github PR, you'll need to manually edit the PR descripton
and commit message for the changes to be reflected here.

Thank you again and I look forward to interacting with you more here!

Please don’t reply on this GitHub thread. Visit golang.org/cl/224897.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-04-08T21:10:42Z

Message from Emmanuel Odeke:

Patch Set 1:

Pierre, please also rebase your change with the latest from master.

Please don’t reply on this GitHub thread. Visit golang.org/cl/224897.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-04-09T14:56:42Z

This PR (HEAD: 971136b) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/224897 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2020-04-09T15:07:19Z

This PR (HEAD: 3f4b10e) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/224897 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2020-04-09T15:11:25Z

This PR (HEAD: f6dda0a) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/224897 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2020-04-09T15:12:55Z

This PR (HEAD: d2f204c) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/224897 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

piec · 2020-04-09T15:16:03Z

Hi @odeke-em, I think I've treated more or less all of your review :)

gopherbot · 2020-04-09T17:48:08Z

Message from Emmanuel Odeke:

Patch Set 8: Run-TryBot+1 Code-Review+2

(24 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/224897.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-04-09T17:49:28Z

Message from Gobot Gobot:

Patch Set 8:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=cd25068e

Please don’t reply on this GitHub thread. Visit golang.org/cl/224897.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-04-09T17:56:03Z

Message from Gobot Gobot:

Patch Set 8:

Build is still in progress...
This change failed on js-wasm:
See https://storage.googleapis.com/go-build-log/cd25068e/js-wasm_391b5774.log

Other builds still in progress; subsequent failure notices suppressed until final report. Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.

Please don’t reply on this GitHub thread. Visit golang.org/cl/224897.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-04-09T18:00:52Z

Message from Gobot Gobot:

Patch Set 8: TryBot-Result-1

1 of 20 TryBots failed:
Failed on js-wasm: https://storage.googleapis.com/go-build-log/cd25068e/js-wasm_391b5774.log

Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.

Please don’t reply on this GitHub thread. Visit golang.org/cl/224897.
After addressing review feedback, remember to publish your drafts!

piec · 2020-04-10T10:01:05Z

Is the goroutine scheduling different on the wasm runtime?
I'll think about making the test less sensitive to how coroutines are scheduled

gopherbot · 2020-04-22T05:25:14Z

Message from Emmanuel Odeke:

Patch Set 8: -Code-Review

Kind ping Pierre, it's been 3 weeks since last ping and the Go tree closes in 10 days or so.

Please don’t reply on this GitHub thread. Visit golang.org/cl/224897.
After addressing review feedback, remember to publish your drafts!

piec · 2020-04-23T14:16:53Z

Hi @odeke-em,
I think the current implementation of the test is sensitive to race conditions, we have no guarantee that the coroutine that handles the cancelation actually gets some time to run. I don't know how the go runtime schedules coroutines on wasm but I suppose it's quite different from other targets and shows this issue even more.
I originally implemented the test with a timeout the same way as TestReverseProxyCancelation to be sure that all coroutines are given a chance to run to exhaustion and the connection is actually canceled. If the implementation is buggy then we'll hit the timeout otherwise the test runs in a fraction of second as usual.
I think I'll push a modified version of the test with the timeout, what do you think?

gopherbot · 2020-04-23T14:41:42Z

This PR (HEAD: 4d313e3) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/224897 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

piec · 2020-04-23T14:45:46Z

Tested locally with (GOOS=linux, GOARCH=amd64) and (GOOS=js, GOARCH=wasm).
It seems fine to me :)
(It's also rebased on master)

gopherbot · 2020-04-23T14:56:03Z

Message from Pierre Carru:

Patch Set 9:

Hi Emmanuel,

I'm not sure that you read the original github issue so I'm copying:

I think the current implementation (patchset 8) of the test is sensitive to race conditions, we have no guarantee that the coroutine that handles the cancelation actually gets some time to run. I don't know how the go runtime schedules coroutines on wasm but I suppose it's quite different from other targets and shows this issue even more.
I originally implemented the test with a timeout the same way as TestReverseProxyCancelation to be sure that all coroutines are given a chance to run to exhaustion and the connection is actually canceled. If the implementation is buggy then we'll hit the timeout otherwise the test runs in a fraction of second as usual.
I think I'll push a modified version of the test with the timeout, what do you think?

Tested locally (patchset 9) with (GOOS=linux, GOARCH=amd64) and (GOOS=js, GOARCH=wasm).
It seems fine to me :)
(It's also rebased on master)

Please don’t reply on this GitHub thread. Visit golang.org/cl/224897.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-04-23T14:57:32Z

Message from Brad Fitzpatrick:

Patch Set 9:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/224897.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-04-23T15:18:39Z

This PR (HEAD: 7747606) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/224897 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2020-04-23T15:22:51Z

Message from Pierre Carru:

Patch Set 10:

I think that should do it

Please don’t reply on this GitHub thread. Visit golang.org/cl/224897.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-04-23T15:41:46Z

Message from Emmanuel Odeke:

Patch Set 10: Run-TryBot+1

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/224897.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-04-23T15:41:47Z

Message from Gobot Gobot:

Patch Set 10:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=622f2854

Please don’t reply on this GitHub thread. Visit golang.org/cl/224897.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-04-23T15:55:22Z

Message from Gobot Gobot:

Patch Set 10: TryBot-Result+1

TryBots are happy.

Please don’t reply on this GitHub thread. Visit golang.org/cl/224897.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-04-23T16:02:50Z

Message from Emmanuel Odeke:

Patch Set 10: Code-Review+2

(1 comment)

Thank you Pierre, just one more change and we’ll be good to go.

Brad, after my suggested naming change, does this look good to you?

RELNOTE=yes

Please don’t reply on this GitHub thread. Visit golang.org/cl/224897.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-04-23T16:22:35Z

This PR (HEAD: 72dd144) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/224897 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2020-04-23T16:32:19Z

Message from Pierre Carru:

Patch Set 11:

OK, just did it

--
By the way what's your opinion about the channel, I did the signaling by closing the channel, but is it not less readable than putting a value?

I mean close(backConnCloseCh) vs backConnCloseCh <- true

Please don’t reply on this GitHub thread. Visit golang.org/cl/224897.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-04-25T14:41:26Z

Message from Emmanuel Odeke:

Patch Set 11:

(1 comment)

Patch Set 11:

OK, just did it

--
By the way what's your opinion about the channel, I did the signaling by closing the channel, but is it not less readable than putting a value?

I mean close(backConnCloseCh) vs backConnCloseCh <- true

close is more idiomatic and ensures that the channel won’t be laying around and will always be closed so any future receives will be immediate.

There is still unaddressed feedback. Please take a look.

Please don’t reply on this GitHub thread. Visit golang.org/cl/224897.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-04-26T09:11:44Z

This PR (HEAD: 3629c78) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/224897 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2020-04-26T09:14:14Z

Message from Pierre Carru:

Patch Set 12:

Oh right I hadn't noticed it

Please don’t reply on this GitHub thread. Visit golang.org/cl/224897.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-04-26T09:15:30Z

Message from Emmanuel Odeke:

Patch Set 12: Run-TryBot+1

Cool, thank you for the update Pierre!

Please don’t reply on this GitHub thread. Visit golang.org/cl/224897.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-04-26T09:15:31Z

Message from Gobot Gobot:

Patch Set 12:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=f30f48ab

Please don’t reply on this GitHub thread. Visit golang.org/cl/224897.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-04-26T09:27:15Z

Message from Gobot Gobot:

Patch Set 12: TryBot-Result+1

TryBots are happy.

Please don’t reply on this GitHub thread. Visit golang.org/cl/224897.
After addressing review feedback, remember to publish your drafts!

… cancelable Ensures that a canceled client request for Switching Protocols (e.g. h2c, Websockets) will cause the underlying connection to be terminated. Adds a goroutine in handleUpgradeResponse in order to select on the incoming client request's context and appropriately cancel it. Fixes #35559 Change-Id: I1238e18fd4cce457f034f78d9cdce0e7f93b8bf6 GitHub-Last-Rev: 3629c78 GitHub-Pull-Request: #38021 Reviewed-on: https://go-review.googlesource.com/c/go/+/224897 Run-TryBot: Emmanuel Odeke <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Emmanuel Odeke <[email protected]>

gopherbot · 2020-04-26T09:29:53Z

Message from Emmanuel Odeke:

Patch Set 12:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/224897.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2020-04-26T09:34:29Z

This PR is being closed because golang.org/cl/224897 has been merged.

… cancelable Ensures that a canceled client request for Switching Protocols (e.g. h2c, Websockets) will cause the underlying connection to be terminated. Adds a goroutine in handleUpgradeResponse in order to select on the incoming client request's context and appropriately cancel it. Fixes golang#35559 Change-Id: I1238e18fd4cce457f034f78d9cdce0e7f93b8bf6 GitHub-Last-Rev: 3629c78 GitHub-Pull-Request: golang#38021 Reviewed-on: https://go-review.googlesource.com/c/go/+/224897 Run-TryBot: Emmanuel Odeke <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Emmanuel Odeke <[email protected]>

googlebot added the cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. label Mar 23, 2020

piec force-pushed the ws-cancel-pr branch from f69f4f9 to 971136b Compare April 9, 2020 14:56

piec force-pushed the ws-cancel-pr branch from 971136b to 3f4b10e Compare April 9, 2020 15:06

piec force-pushed the ws-cancel-pr branch from 3f4b10e to f6dda0a Compare April 9, 2020 15:10

piec force-pushed the ws-cancel-pr branch from f6dda0a to d2f204c Compare April 9, 2020 15:12

odeke-em changed the title ~~net/http/httputil: make handleUpgradeResponse cancelable~~ net/http/httputil: make Switching Protocol requests (e.g. Websockets) cancelable Apr 9, 2020

piec force-pushed the ws-cancel-pr branch from 2bc6618 to 4d313e3 Compare April 23, 2020 14:40

piec force-pushed the ws-cancel-pr branch from 4d313e3 to 7747606 Compare April 23, 2020 15:17

piec force-pushed the ws-cancel-pr branch from 7747606 to 72dd144 Compare April 23, 2020 16:21

piec added 2 commits April 26, 2020 11:09

net/http/httputil: make handleUpgradeResponse cancelable

ae44b42

net/http/httputil: TestReverseProxyWebSocketCancelation

3629c78

piec force-pushed the ws-cancel-pr branch from 72dd144 to 3629c78 Compare April 26, 2020 09:10

gopherbot closed this Apr 26, 2020

net/http/httputil: make Switching Protocol requests (e.g. Websockets) cancelable #38021

net/http/httputil: make Switching Protocol requests (e.g. Websockets) cancelable #38021

Conversation

piec commented Mar 23, 2020 • edited by odeke-em Loading

gopherbot commented Mar 23, 2020

gopherbot commented Mar 23, 2020

gopherbot commented Mar 23, 2020

gopherbot commented Apr 8, 2020

gopherbot commented Apr 9, 2020

gopherbot commented Apr 9, 2020

gopherbot commented Apr 9, 2020

gopherbot commented Apr 9, 2020

piec commented Apr 9, 2020

gopherbot commented Apr 9, 2020

gopherbot commented Apr 9, 2020

gopherbot commented Apr 9, 2020

gopherbot commented Apr 9, 2020

piec commented Apr 10, 2020

gopherbot commented Apr 22, 2020

piec commented Apr 23, 2020

gopherbot commented Apr 23, 2020

piec commented Apr 23, 2020 • edited Loading

gopherbot commented Apr 23, 2020

gopherbot commented Apr 23, 2020

gopherbot commented Apr 23, 2020

gopherbot commented Apr 23, 2020

gopherbot commented Apr 23, 2020

gopherbot commented Apr 23, 2020

gopherbot commented Apr 23, 2020

gopherbot commented Apr 23, 2020

gopherbot commented Apr 23, 2020

gopherbot commented Apr 23, 2020

gopherbot commented Apr 25, 2020

gopherbot commented Apr 26, 2020

gopherbot commented Apr 26, 2020

gopherbot commented Apr 26, 2020

gopherbot commented Apr 26, 2020

gopherbot commented Apr 26, 2020

gopherbot commented Apr 26, 2020

gopherbot commented Apr 26, 2020

piec commented Mar 23, 2020 •

edited by odeke-em

Loading

piec commented Apr 23, 2020 •

edited

Loading