net/http: never cancel r.Context() after Hijack #32315

nhooyr · 2019-05-29T23:48:42Z

gopherbot · 2019-05-29T23:49:48Z

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

Please visit https://go-review.googlesource.com/c/go/+/179458 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 · 2019-05-29T23:51:40Z

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

Please visit https://go-review.googlesource.com/c/go/+/179458 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 · 2019-05-30T09:42:39Z

Message from Dave Cheney:

Patch Set 2:

Thank you for this change. Please add a test so this does not regress in the future.

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

gopherbot · 2019-05-30T18:07:43Z

Message from Brad Fitzpatrick:

Patch Set 2: Run-TryBot+1

This looks fine, but do please add a new test.

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

gopherbot · 2019-05-30T18:07:44Z

Message from Gobot Gobot:

Patch Set 2:

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

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

gopherbot · 2019-05-30T18:14:20Z

Message from Gobot Gobot:

Patch Set 2:

Build is still in progress...
This change failed on misc-compile-ppc:
See https://storage.googleapis.com/go-build-log/0e59d0ea/misc-compile-ppc_7d7cd769.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/179458.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-05-30T18:23:47Z

Message from Brad Fitzpatrick:

Patch Set 3: Patch Set 2 was rebased

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

gopherbot · 2019-05-30T18:23:48Z

Message from Brad Fitzpatrick:

Patch Set 3: Run-TryBot+1

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

gopherbot · 2019-05-30T18:23:49Z

Message from Gobot Gobot:

Patch Set 2: TryBot-Result-1

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/179458.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-05-30T18:23:51Z

Message from Gobot Gobot:

Patch Set 3:

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

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

gopherbot · 2019-05-30T18:31:17Z

Message from Gobot Gobot:

Patch Set 3:

Build is still in progress...
This change failed on openbsd-amd64-64:
See https://storage.googleapis.com/go-build-log/f804102c/openbsd-amd64-64_f935d8de.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/179458.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-05-30T18:34:48Z

Message from Gobot Gobot:

Patch Set 3: TryBot-Result-1

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/179458.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-06-10T12:33:54Z

Message from Emmanuel Odeke:

Patch Set 3:

Patch Set 2: Run-TryBot+1

This looks fine, but do please add a new test.

Anmol, perhaps https://play.golang.org/p/xFSvh6j_h7F might be a test for you or inlined below:

package main

import (
"context"
"io/ioutil"
"net/http"
"net/http/httptest"
"strings"
"testing"
)

func TestConnCloseNoReqCancellation(t *testing.T) {
cst := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
hj, ok := w.(http.Hijacker)
defer func() {
ctx := r.Context()
select {
case <-ctx.Done():
if err := ctx.Err(); err == context.Canceled {
t.Fatalf("client.Context was canceled: %v", err)
}
default:
}
}()
if !ok {
w.WriteHeader(http.StatusNotImplemented)
return
}
conn, bufrw, err := hj.Hijack()
if err != nil {
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
conn.Close()

	_, _ = ioutil.ReadAll(bufrw)
	bufrw.Write([]byte("HTTP/1.1 200 OK\r\nConnection: keep-alive\r\nContent-Length: 2\r\n\r\nok"))
	bufrw.Flush()
}))
defer cst.Close()

req, _ := http.NewRequest("POST", cst.URL, strings.NewReader("aaaaaaa*****aaaaaaaaaaaa"))
res, _ := cst.Client().Do(req)
if res != nil {
	_, _ = ioutil.ReadAll(res.Body)
	_ = res.Body.Close()
}

}

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

gopherbot · 2019-06-10T17:17:04Z

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

Please visit https://go-review.googlesource.com/c/go/+/179458 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

Closes #32314

gopherbot · 2019-06-10T17:19:32Z

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

Please visit https://go-review.googlesource.com/c/go/+/179458 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 · 2019-06-10T17:28:35Z

Message from Anmol Sethi:

Patch Set 4:

I have added a test.

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

gopherbot · 2019-06-11T04:46:56Z

Message from Emmanuel Odeke:

Patch Set 5: Run-TryBot+1

(10 comments)

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

gopherbot · 2019-06-11T04:46:57Z

Message from Gobot Gobot:

Patch Set 5:

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

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

gopherbot · 2019-06-11T04:53:42Z

Message from Gobot Gobot:

Patch Set 5:

Build is still in progress...
This change failed on js-wasm:
See https://storage.googleapis.com/go-build-log/43576df4/js-wasm_3ba650b2.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/179458.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-06-11T04:59:20Z

Message from Gobot Gobot:

Patch Set 5: TryBot-Result-1

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/179458.
After addressing review feedback, remember to publish your drafts!

nhooyr · 2019-06-11T13:39:08Z

@odeke-em feel free to take over, I don't have the time to respond to the reviews

odeke-em · 2019-06-11T16:45:13Z

Hey @nhyor, I think you copied the test that I provided verbatim, it won't have the trybot failures and also matches the style for code review :) Just that change and it'll be perhaps good to go.

…

On Tue, Jun 11, 2019 at 6:42 AM Anmol Sethi ***@***.***> wrote: Closed #32315 <#32315>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#32315?email_source=notifications&email_token=ABFL3VZ656MJNHZNADRUMX3PZ6TVDA5CNFSM4HQYGVV2YY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOR5GO65Q#event-2404183926>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABFL3V27XU3UN2DDEFQ3CSDPZ6TVDANCNFSM4HQYGVVQ> .

odeke-em · 2019-06-11T16:45:52Z

*I think if you copy the test that I provided verbatim Sorry was typing from my phone

…

On Tue, Jun 11, 2019 at 9:44 AM Emmanuel Odeke ***@***.***> wrote: Hey @nhyor, I think you copied the test that I provided verbatim, it won't have the trybot failures and also matches the style for code review :) Just that change and it'll be perhaps good to go. On Tue, Jun 11, 2019 at 6:42 AM Anmol Sethi ***@***.***> wrote: > Closed #32315 <#32315>. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#32315?email_source=notifications&email_token=ABFL3VZ656MJNHZNADRUMX3PZ6TVDA5CNFSM4HQYGVV2YY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOR5GO65Q#event-2404183926>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ABFL3V27XU3UN2DDEFQ3CSDPZ6TVDANCNFSM4HQYGVVQ> > . >

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 May 29, 2019

nhooyr changed the title ~~net/http: never ever cancel r.Context() after Hijack~~ net/http: never cancel r.Context() after Hijack May 29, 2019

net/http: never cancel r.Context() after Hijack

14b4712

Closes #32314

nhooyr closed this Jun 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net/http: never cancel r.Context() after Hijack #32315

net/http: never cancel r.Context() after Hijack #32315

nhooyr commented May 29, 2019

gopherbot commented May 29, 2019

gopherbot commented May 29, 2019

gopherbot commented May 30, 2019

gopherbot commented May 30, 2019

gopherbot commented May 30, 2019

gopherbot commented May 30, 2019

gopherbot commented May 30, 2019

gopherbot commented May 30, 2019

gopherbot commented May 30, 2019

gopherbot commented May 30, 2019

gopherbot commented May 30, 2019

gopherbot commented May 30, 2019

gopherbot commented Jun 10, 2019

gopherbot commented Jun 10, 2019

gopherbot commented Jun 10, 2019

gopherbot commented Jun 10, 2019

gopherbot commented Jun 11, 2019

gopherbot commented Jun 11, 2019

gopherbot commented Jun 11, 2019

gopherbot commented Jun 11, 2019

nhooyr commented Jun 11, 2019

odeke-em commented Jun 11, 2019 via email

odeke-em commented Jun 11, 2019 via email

net/http: never cancel r.Context() after Hijack #32315

net/http: never cancel r.Context() after Hijack #32315

Conversation

nhooyr commented May 29, 2019

gopherbot commented May 29, 2019

gopherbot commented May 29, 2019

gopherbot commented May 30, 2019

gopherbot commented May 30, 2019

gopherbot commented May 30, 2019

gopherbot commented May 30, 2019

gopherbot commented May 30, 2019

gopherbot commented May 30, 2019

gopherbot commented May 30, 2019

gopherbot commented May 30, 2019

gopherbot commented May 30, 2019

gopherbot commented May 30, 2019

gopherbot commented Jun 10, 2019

gopherbot commented Jun 10, 2019

gopherbot commented Jun 10, 2019

gopherbot commented Jun 10, 2019

gopherbot commented Jun 11, 2019

gopherbot commented Jun 11, 2019

gopherbot commented Jun 11, 2019

gopherbot commented Jun 11, 2019

nhooyr commented Jun 11, 2019

odeke-em commented Jun 11, 2019 via email

odeke-em commented Jun 11, 2019 via email