|
|
Descriptionnet/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/ #MessagesTotal messages: 27
Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
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... src/pkg/net/http/server.go:1601: ClientConnState func(net.Conn, ConnectionState) Perhaps just ConnState? Not obvious to me what else it could be confused with. https://codereview.appspot.com/69260044/diff/20001/src/pkg/net/http/server.go... src/pkg/net/http/server.go:1604: // A ConnectionState represents the The second half of this comment is missing. https://codereview.appspot.com/69260044/diff/20001/src/pkg/net/http/server.go... src/pkg/net/http/server.go:1605: type ConnectionState int Maybe just ConnState, to match net.Conn and ClientConnState above? https://codereview.appspot.com/69260044/diff/20001/src/pkg/net/http/server.go... src/pkg/net/http/server.go:1612: // StateReading represent a connection that is reading its request. s/represent/represents/ s/.// https://codereview.appspot.com/69260044/diff/20001/src/pkg/net/http/server.go... src/pkg/net/http/server.go:1615: Seems like there's a missing state here, StateHandler(?). // StateHandler represents a connection that has read its request and is in a Handler. Unless I'm missing something, without such a state, the connection state watcher can't distinguish between a state stuck waiting for a request read to complete and a state stuck in a slow Handler. https://codereview.appspot.com/69260044/diff/20001/src/pkg/net/http/server.go... src/pkg/net/http/server.go:1616: // StateIdle represents a connection that has finished a request s/finished a request/finished handling a request/
Sign in to reply to this message.
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... src/pkg/net/http/server.go:1601: ClientConnState func(net.Conn, ConnectionState) On 2014/02/26 23:07:56, josharian wrote: > Perhaps just ConnState? Not obvious to me what else it could be confused with. I'm okay with that too, but there's also precedent for my naming: http://golang.org/pkg/crypto/tls/#ConnectionState But that's a struct anyway. And if we go with ConnState, then maybe we could re-use it for state-tracking conns in the HTTP client too (unlikely). https://codereview.appspot.com/69260044/diff/20001/src/pkg/net/http/server.go... src/pkg/net/http/server.go:1605: type ConnectionState int On 2014/02/26 23:07:56, josharian wrote: > Maybe just ConnState, to match net.Conn and ClientConnState above? See above. https://codereview.appspot.com/69260044/diff/20001/src/pkg/net/http/server.go... src/pkg/net/http/server.go:1612: // StateReading represent a connection that is reading its request. On 2014/02/26 23:07:56, josharian wrote: > s/represent/represents/ > s/.// Done. https://codereview.appspot.com/69260044/diff/20001/src/pkg/net/http/server.go... src/pkg/net/http/server.go:1615: On 2014/02/26 23:07:56, josharian wrote: > Seems like there's a missing state here, StateHandler(?). > > // StateHandler represents a connection that has read its request and is in a > Handler. > > Unless I'm missing something, without such a state, the connection state watcher > can't distinguish between a state stuck waiting for a request read to complete > and a state stuck in a slow Handler. I'd rather not introduce that. That's pretty much what this state already means. The two seem redundant. I've updated the comment instead. https://codereview.appspot.com/69260044/diff/20001/src/pkg/net/http/server.go... src/pkg/net/http/server.go:1616: // StateIdle represents a connection that has finished a request On 2014/02/26 23:07:56, josharian wrote: > s/finished a request/finished handling a request/ Done.
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, josharian@gmail.com (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
I guess not "all" done, since I didn't do the Handler state. See comment. On Wed, Feb 26, 2014 at 3:16 PM, <bradfitz@golang.org> wrote: > Hello golang-codereviews@googlegroups.com, josharian@gmail.com (cc: > golang-codereviews@googlegroups.com), > > Please take another look. > > > https://codereview.appspot.com/69260044/ >
Sign in to reply to this message.
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... src/pkg/net/http/server.go:1601: ClientConnState func(net.Conn, ConnectionState) On 2014/02/26 23:16:43, bradfitz wrote: > On 2014/02/26 23:07:56, josharian wrote: > > Perhaps just ConnState? Not obvious to me what else it could be confused with. > > I'm okay with that too, but there's also precedent for my naming: > http://golang.org/pkg/crypto/tls/#ConnectionState > > But that's a struct anyway. And if we go with ConnState, then maybe we could > re-use it for state-tracking conns in the HTTP client too (unlikely). Sorry, I was unclear. I was suggesting s/ClientConnState/ConnState/ here. https://codereview.appspot.com/69260044/diff/40001/src/pkg/net/http/server.go File src/pkg/net/http/server.go (right): https://codereview.appspot.com/69260044/diff/40001/src/pkg/net/http/server.go... src/pkg/net/http/server.go:1615: // and is about to be in a Handler if the request is valid. If the request is not valid, will it always transition to StateClosed, or might it transition to StateIdle? Can it transition directly back to StateReading again, e.g. in the case of a 100 Continue? Generally, it'd be nice to be able to infer the entire state machine from the docs, since understanding the available transitions will be important to using this correctly.
Sign in to reply to this message.
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 src/pkg/net/http/server.go (right): https://codereview.appspot.com/69260044/diff/20001/src/pkg/net/http/server.go... src/pkg/net/http/server.go:1622: // StateClose. s/StateClose/StateClosed/ https://codereview.appspot.com/69260044/diff/20001/src/pkg/net/http/server.go... src/pkg/net/http/server.go:1626: // Hijacked connections do not transition to StateClose. s/StateClose/StateClosed/
Sign in to reply to this message.
Thanks. Please report back soon--- the window for new stuff soon. On Wed, Feb 26, 2014 at 5:44 PM, <r@rcrowley.org> wrote: > 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 src/pkg/net/http/server.go (right): > > https://codereview.appspot.com/69260044/diff/20001/src/ > pkg/net/http/server.go#newcode1622 > src/pkg/net/http/server.go:1622: // StateClose. > s/StateClose/StateClosed/ > > https://codereview.appspot.com/69260044/diff/20001/src/ > pkg/net/http/server.go#newcode1626 > src/pkg/net/http/server.go:1626: // Hijacked connections do not > transition to StateClose. > s/StateClose/StateClosed/ > > https://codereview.appspot.com/69260044/ >
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, josharian@gmail.com, r@rcrowley.org (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
Josh, Richard, et al, PTAL. I've updated the state/transition docs and renamed the hook per Josh's feedback. On Wed, Feb 26, 2014 at 8:01 PM, <bradfitz@golang.org> wrote: > Hello golang-codereviews@googlegroups.com, josharian@gmail.com, > r@rcrowley.org (cc: golang-codereviews@googlegroups.com), > > Please take another look. > > > https://codereview.appspot.com/69260044/ >
Sign in to reply to this message.
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... src/pkg/net/http/server.go:1608: const ( It's not clear to me from these docs what state a connection is in when it is being handled or is that not possible?
Sign in to reply to this message.
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... src/pkg/net/http/server.go:1608: const ( On 2014/02/27 04:54:14, adg wrote: > It's not clear to me from these docs what state a connection is in when it is > being handled > or is that not possible? Like I said to Josh earlier, I don't want to introduce that state. Read the docs on StateReading again, though. It says the transition from StateReading through a handler (which has no state of its own) and then to one of the other three states. Reasons I don't want to add that state: 1) Once data is flowing, the client has expectations of an answer. There's basically nothing a client could usefully do between StateReading and a hypothetical in-handler state. 2) The client using the hooks can already know when something's in a Handler--- they control the Handler anyway, and they're going to have to use it anyway to set the "Connection: close" header for nice shutdown anyway. 3) The transition from StateReading to hypothetical state-handling would be almost always immediately after each other, since the HTTP request typically all comes in a packet. Would you and Josh prefer the name "StateActive" to "StateReading"? Then the state machine is: New -> Active Active -> Closed Active -> Idle Active -> Hijacked Idle -> Active I could clarify the docs on StateReading to say the callback happens before it enters a Handler, but it remains in the Active / Reading state until the Handler completes.
Sign in to reply to this message.
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... src/pkg/net/http/server.go:1091: c.setState(origConn, StateNew) There's a race here if the user's using a sync.WaitGroup where a connection being created as the graceful stop begins could be missed by sync.WaitGroup.Wait and allow the process to exit early. I think moving this above go c.serve() should suffice.
Sign in to reply to this message.
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 would close listener, atomically set a global int32 to 1, wait a second, then for any new conns entering idle, set their read delay to 5 seconds. On active, set to 30 seconds to not kill ones in flight. In handler, check atomic int32 and set Connection close. With custom listener, wait for all to be closed. You don't even need the hook to look at any state other than idle and active. Maybe you want to break long-running hijacked things (web socket handlers). On Feb 27, 2014 12:25 AM, <r@rcrowley.org> wrote: > > 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 using a sync.WaitGroup where a > connection being created as the graceful stop begins could be missed by > sync.WaitGroup.Wait and allow the process to exit early. I think moving > this above > > go c.serve() > > should suffice. > > https://codereview.appspot.com/69260044/ >
Sign in to reply to this message.
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 at 12:33 PM, Brad Fitzpatrick <bradfitz@golang.org> 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 would close listener, atomically set a global int32 to 1, wait a second, > then for any new conns entering idle, set their read delay to 5 seconds. On > active, set to 30 seconds to not kill ones in flight. In handler, check > atomic int32 and set Connection close. With custom listener, wait for all to > be closed. You don't even need the hook to look at any state other than idle > and active. Maybe you want to break long-running hijacked things (web socket > handlers). > > On Feb 27, 2014 12:25 AM, <r@rcrowley.org> wrote: >> >> >> >> 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... >> src/pkg/net/http/server.go:1091: c.setState(origConn, StateNew) >> There's a race here if the user's using a sync.WaitGroup where a >> connection being created as the graceful stop begins could be missed by >> sync.WaitGroup.Wait and allow the process to exit early. I think moving >> this above >> >> go c.serve() >> >> should suffice. >> >> https://codereview.appspot.com/69260044/ > > -- > You received this message because you are subscribed to the Google Groups > "golang-codereviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-codereviews+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/groups/opt_out.
Sign in to reply to this message.
On 2014/02/27 03:55:56, bradfitz wrote: > Thanks. Please report back soon--- the window for new stuff soon. > > On Wed, Feb 26, 2014 at 5:44 PM, <mailto:r@rcrowley.org> wrote: > > > This looks sort of promising. I'll take a stab at using it tonight. > > Good: with ConnState I no longer need to augment net.Conn and net.Listener with a sync.WaitGroup. Neutral: I still need to augment the root http.Handler in order to augment the root http.ResponseWriter to add the Connection: close header. Bad: there's still a problematic series of events that make gracefully shutting down keepalive connections impractical without a very short ReadTimeout. 1. Accept returns a net.Conn; ConnState(c, StateNew) 2. readRequest, etc; ConnState(c, StateReading) 3. Responded; ConnState(c, StateIdle) 4. SIGQUIT 5. ReadTimeout finally happens; ConnState(c, StateClosed) 6. It's safe to exit now that all connections are closed. The time between steps 4 and 5 should be minimized for a graceful stop to be useful. A naive implementation maintains a map of connections that have entered StateIdle, deletes connections from the map should they transition back to StateReading, and closes all the connections in the map some short time after initiating graceful stop.
Sign in to reply to this message.
Are you against the map tracking the set of idle conns? Then in 5 seconds, close anything still there that was there 5 seconds ago. Things transitioning to idle after SIGQUIT will get a few seconds read time out set of them so they'll die naturally. On Feb 27, 2014 12:50 AM, <r@rcrowley.org> wrote: > On 2014/02/27 03:55:56, bradfitz wrote: > >> Thanks. Please report back soon--- the window for new stuff soon. >> > > On Wed, Feb 26, 2014 at 5:44 PM, <mailto:r@rcrowley.org> wrote: >> > > > This looks sort of promising. I'll take a stab at using it tonight. >> > >> > > Good: with ConnState I no longer need to augment net.Conn and > net.Listener with a sync.WaitGroup. > > Neutral: I still need to augment the root http.Handler in order to > augment the root http.ResponseWriter to add the Connection: close > header. > > Bad: there's still a problematic series of events that make gracefully > shutting down keepalive connections impractical without a very short > ReadTimeout. > > 1. Accept returns a net.Conn; ConnState(c, StateNew) > 2. readRequest, etc; ConnState(c, StateReading) > 3. Responded; ConnState(c, StateIdle) > 4. SIGQUIT > 5. ReadTimeout finally happens; ConnState(c, StateClosed) > 6. It's safe to exit now that all connections are closed. > > The time between steps 4 and 5 should be minimized for a graceful stop > to be useful. A naive implementation maintains a map of connections > that have entered StateIdle, deletes connections from the map should > they transition back to StateReading, and closes all the connections in > the map some short time after initiating graceful stop. > > https://codereview.appspot.com/69260044/ >
Sign in to reply to this message.
> Would you and Josh prefer the name "StateActive" to "StateReading"? I would. > state machine is: > > New -> Active > Active -> Closed > Active -> Idle > Active -> Hijacked > Idle -> Active FWIW, this doesn't quite match the current docs. Docs say: "StateReading represents a connection that has read 1 or more bytes of a request but has not yet entered a Handler." However, an idle connection transitions to active and then closed when the remote hangs up, even if the remote does not send any additional requests. That is, if the client makes a single HTTP/1.1 request and then hangs up without sending more data, the states are: new, reading, idle, reading, closed. I'm not sure whether the docs or the behavior are incorrect here. I'm inclined to say the behavior, and that there should be an additional possible transition from Idle -> Closed. > I could clarify the docs on StateReading to say the callback happens before it > enters a Handler, but it remains in the Active / Reading state until the Handler > completes. Good idea.
Sign in to reply to this message.
On 2014/02/27 09:00:48, bradfitz wrote: > Are you against the map tracking the set of idle conns? Against, no. That's what I prototyped out and it does work. Per my comments in 67730046 I think the standard library's in a better position to solve this. Specifically, all it has to keep track of is whether the Server is stopping and the decision as to what to do with each connection is made locally. The synchronized map feels heavy. Let me finish it enough to get to numbers about the performance regression the synchronized map introduces.
Sign in to reply to this message.
Also, go1.3.txt?
Sign in to reply to this message.
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.
Sign in to reply to this message.
PTAL. I'll do a batch of go1.3.txt in a subsequent CL. On Thu, Feb 27, 2014 at 10:13 AM, <josharian@gmail.com> wrote: > Also, go1.3.txt? > > https://codereview.appspot.com/69260044/ >
Sign in to reply to this message.
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... src/pkg/net/http/server.go:1599: // ConnState optionally specifies a callback function ConnState specifies an optional callback function that is called when a client connection changes state. See the ConnState type and associated constants for the details. https://codereview.appspot.com/69260044/diff/70001/src/pkg/net/http/server.go... src/pkg/net/http/server.go:1609: // StateNew represents a new connection that is expected to some of these being with "StateX represents" and others "StateX occurs". Use one (the former I think) https://codereview.appspot.com/69260044/diff/70001/src/pkg/net/http/server.go... src/pkg/net/http/server.go:1610: // send a request immediately. All connections begin at this s/All c/C/ https://codereview.appspot.com/69260044/diff/70001/src/pkg/net/http/server.go... src/pkg/net/http/server.go:1617: // fires before the request has entered a handler and persists what does "persists" mean? the hook persists? what? https://codereview.appspot.com/69260044/diff/70001/src/pkg/net/http/server.go... src/pkg/net/http/server.go:1618: // in StateActive until the request has been handled, Split the sentence: When the request has been handled the connection's state transitions to StateClosed, ....
Sign in to reply to this message.
All done. On Thu, Feb 27, 2014 at 4:13 PM, <adg@golang.org> wrote: > 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 a > callback function > ConnState specifies an optional callback function that is called when a > client connection changes state. See the ConnState type and associated > constants for the details. > > https://codereview.appspot.com/69260044/diff/70001/src/ > pkg/net/http/server.go#newcode1609 > src/pkg/net/http/server.go:1609: // StateNew represents a new connection > that is expected to > some of these being with "StateX represents" and others "StateX occurs". > Use one (the former I think) > > https://codereview.appspot.com/69260044/diff/70001/src/ > pkg/net/http/server.go#newcode1610 > src/pkg/net/http/server.go:1610: // send a request immediately. All > connections begin at this > s/All c/C/ > > https://codereview.appspot.com/69260044/diff/70001/src/ > pkg/net/http/server.go#newcode1617 > src/pkg/net/http/server.go:1617: // fires before the request has entered > a handler and persists > what does "persists" mean? the hook persists? what? > > https://codereview.appspot.com/69260044/diff/70001/src/ > pkg/net/http/server.go#newcode1618 > src/pkg/net/http/server.go:1618: // in StateActive until the request has > been handled, > Split the sentence: When the request has been handled the connection's > state transitions to StateClosed, .... > > https://codereview.appspot.com/69260044/ >
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=7a7a12d7b8c0 *** 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. LGTM=adg R=golang-codereviews, josharian, r, adg, dvyukov CC=golang-codereviews https://codereview.appspot.com/69260044
Sign in to reply to this message.
Message was sent while issue was closed.
This CL appears to have broken the solaris-amd64-solaris11 builder.
Sign in to reply to this message.
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.
|