Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(571)

Issue 69260044: code review 69260044: net/http: add optional Server.ConnState callback (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 1 month ago by bradfitz
Modified:
10 years ago
Reviewers:
gobot, adg, rcrowley
CC:
golang-codereviews, josharian, adg, dvyukov
Visibility:
Public.

Description

net/http: add optional Server.ConnState callback Update Issue 4674 This allows for all sorts of graceful shutdown policies, without picking a policy (e.g. lameduck period) and without adding lots of locking to the server core. That policy and locking can be implemented outside of net/http now.

Patch Set 1 #

Patch Set 2 : diff -r 1424452d7eb5 https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r 1424452d7eb5 https://go.googlecode.com/hg/ #

Total comments: 14

Patch Set 4 : diff -r 1424452d7eb5 https://go.googlecode.com/hg/ #

Total comments: 1

Patch Set 5 : diff -r 1424452d7eb5 https://go.googlecode.com/hg/ #

Total comments: 3

Patch Set 6 : diff -r 9d51d21e0ac6 https://go.googlecode.com/hg/ #

Total comments: 5

Patch Set 7 : diff -r ffcbc9233426 https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+182 lines, -0 lines) Patch
M src/pkg/net/http/serve_test.go View 1 2 3 4 5 2 chunks +115 lines, -0 lines 0 comments Download
M src/pkg/net/http/server.go View 1 2 3 4 5 6 6 chunks +67 lines, -0 lines 0 comments Download

Messages

Total messages: 27
bradfitz
Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
10 years, 1 month ago (2014-02-26 22:23:58 UTC) #1
josharian
I like the approach. I have some nitpicks for you. https://codereview.appspot.com/69260044/diff/20001/src/pkg/net/http/server.go File src/pkg/net/http/server.go (right): https://codereview.appspot.com/69260044/diff/20001/src/pkg/net/http/server.go#newcode1601 ...
10 years, 1 month ago (2014-02-26 23:07:56 UTC) #2
bradfitz
All done. https://codereview.appspot.com/69260044/diff/20001/src/pkg/net/http/server.go File src/pkg/net/http/server.go (right): https://codereview.appspot.com/69260044/diff/20001/src/pkg/net/http/server.go#newcode1601 src/pkg/net/http/server.go:1601: ClientConnState func(net.Conn, ConnectionState) On 2014/02/26 23:07:56, josharian ...
10 years, 1 month ago (2014-02-26 23:16:43 UTC) #3
bradfitz
Hello golang-codereviews@googlegroups.com, josharian@gmail.com (cc: golang-codereviews@googlegroups.com), Please take another look.
10 years, 1 month ago (2014-02-26 23:16:51 UTC) #4
bradfitz
I guess not "all" done, since I didn't do the Handler state. See comment. On ...
10 years, 1 month ago (2014-02-26 23:17:21 UTC) #5
josharian
https://codereview.appspot.com/69260044/diff/20001/src/pkg/net/http/server.go File src/pkg/net/http/server.go (right): https://codereview.appspot.com/69260044/diff/20001/src/pkg/net/http/server.go#newcode1601 src/pkg/net/http/server.go:1601: ClientConnState func(net.Conn, ConnectionState) On 2014/02/26 23:16:43, bradfitz wrote: > ...
10 years, 1 month ago (2014-02-26 23:41:57 UTC) #6
rcrowley
This looks sort of promising. I'll take a stab at using it tonight. https://codereview.appspot.com/69260044/diff/20001/src/pkg/net/http/server.go File ...
10 years, 1 month ago (2014-02-27 01:44:08 UTC) #7
bradfitz
Thanks. Please report back soon--- the window for new stuff soon. On Wed, Feb 26, ...
10 years, 1 month ago (2014-02-27 03:55:56 UTC) #8
bradfitz
Hello golang-codereviews@googlegroups.com, josharian@gmail.com, r@rcrowley.org (cc: golang-codereviews@googlegroups.com), Please take another look.
10 years, 1 month ago (2014-02-27 04:01:25 UTC) #9
bradfitz
Josh, Richard, et al, PTAL. I've updated the state/transition docs and renamed the hook per ...
10 years, 1 month ago (2014-02-27 04:02:09 UTC) #10
adg
https://codereview.appspot.com/69260044/diff/50001/src/pkg/net/http/server.go File src/pkg/net/http/server.go (right): https://codereview.appspot.com/69260044/diff/50001/src/pkg/net/http/server.go#newcode1608 src/pkg/net/http/server.go:1608: const ( It's not clear to me from these ...
10 years, 1 month ago (2014-02-27 04:54:14 UTC) #11
bradfitz
https://codereview.appspot.com/69260044/diff/50001/src/pkg/net/http/server.go File src/pkg/net/http/server.go (right): https://codereview.appspot.com/69260044/diff/50001/src/pkg/net/http/server.go#newcode1608 src/pkg/net/http/server.go:1608: const ( On 2014/02/27 04:54:14, adg wrote: > It's ...
10 years, 1 month ago (2014-02-27 07:07:50 UTC) #12
rcrowley
https://codereview.appspot.com/69260044/diff/50001/src/pkg/net/http/server.go File src/pkg/net/http/server.go (right): https://codereview.appspot.com/69260044/diff/50001/src/pkg/net/http/server.go#newcode1091 src/pkg/net/http/server.go:1091: c.setState(origConn, StateNew) There's a race here if the user's ...
10 years, 1 month ago (2014-02-27 08:25:34 UTC) #13
bradfitz
I don't see the difference with moving it. I prefer them all in the same ...
10 years, 1 month ago (2014-02-27 08:33:43 UTC) #14
dvyukov
I think it's the same low-level race as here: https://code.google.com/p/go/issues/detail?id=7264 On Thu, Feb 27, 2014 ...
10 years, 1 month ago (2014-02-27 08:38:05 UTC) #15
rcrowley
On 2014/02/27 03:55:56, bradfitz wrote: > Thanks. Please report back soon--- the window for new ...
10 years, 1 month ago (2014-02-27 08:50:44 UTC) #16
bradfitz
Are you against the map tracking the set of idle conns? Then in 5 seconds, ...
10 years, 1 month ago (2014-02-27 09:00:48 UTC) #17
josharian
> Would you and Josh prefer the name "StateActive" to "StateReading"? I would. > state ...
10 years, 1 month ago (2014-02-27 15:39:07 UTC) #18
rcrowley
On 2014/02/27 09:00:48, bradfitz wrote: > Are you against the map tracking the set of ...
10 years, 1 month ago (2014-02-27 15:47:54 UTC) #19
josharian
Also, go1.3.txt?
10 years, 1 month ago (2014-02-27 18:13:24 UTC) #20
bradfitz
Hello golang-codereviews@googlegroups.com, josharian@gmail.com, r@rcrowley.org, adg@golang.org, dvyukov@google.com (cc: golang-codereviews@googlegroups.com), Please take another look.
10 years, 1 month ago (2014-02-27 20:52:07 UTC) #21
bradfitz
PTAL. I'll do a batch of go1.3.txt in a subsequent CL. On Thu, Feb 27, ...
10 years, 1 month ago (2014-02-27 20:52:39 UTC) #22
adg
LGTM functionally, some doc nits https://codereview.appspot.com/69260044/diff/70001/src/pkg/net/http/server.go File src/pkg/net/http/server.go (right): https://codereview.appspot.com/69260044/diff/70001/src/pkg/net/http/server.go#newcode1599 src/pkg/net/http/server.go:1599: // ConnState optionally specifies ...
10 years, 1 month ago (2014-02-28 00:13:23 UTC) #23
bradfitz
All done. On Thu, Feb 27, 2014 at 4:13 PM, <adg@golang.org> wrote: > LGTM functionally, ...
10 years, 1 month ago (2014-02-28 02:24:11 UTC) #24
bradfitz
*** Submitted as https://code.google.com/p/go/source/detail?r=7a7a12d7b8c0 *** net/http: add optional Server.ConnState callback Update Issue 4674 This allows ...
10 years, 1 month ago (2014-02-28 02:29:07 UTC) #25
gobot
This CL appears to have broken the solaris-amd64-solaris11 builder.
10 years, 1 month ago (2014-02-28 03:30:08 UTC) #26
rcrowley
10 years ago (2014-03-01 23:10:47 UTC) #27
Message was sent while issue was closed.
On 2014/02/27 08:33:43, bradfitz wrote:
> I don't see the difference with moving it. I prefer them all in the same
> func.
> 
> Describe the race again? I understand it's not a data race but some higher
> level race, but I'm not sure what you're doing. Code?

I sent out https://codereview.appspot.com/70410044 with much more explanation.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b