api/state: implement Server.Stop

api/state: implement Server.Stop

This enables us to tear down everything cleanly and restart
when mongodb goes down.

https://code.launchpad.net/~rogpeppe/juju-core/179-server-stop/+merge/139038

(do not edit description out of merge proposal)

[rog, 2012-12-10 17:05:52]

Please take a look.

[dave_cheney.net, 2012-12-10 21:53:06]

LGTM. Some minor suggestions.

https://codereview.appspot.com/6902070/diff/2001/state/api/api_test.go
File state/api/api_test.go (right):

https://codereview.appspot.com/6902070/diff/2001/state/api/api_test.go#newcode46
state/api/api_test.go:46: func (s *suite) TearDownTest(c *C) {
if s.srv is nil, from the SetUpTest failing above, this may panic.

https://codereview.appspot.com/6902070/diff/2001/state/api/api_test.go#newcode76
state/api/api_test.go:76: c.Assert(err, ErrorMatches, "cannot receive response:
EOF")
This is slightly fragile, net.Conn can return a number of other err responses
during the closing phase. EOF is probably a safe default.

https://codereview.appspot.com/6902070/diff/2001/state/api/serve.go
File state/api/serve.go (right):

https://codereview.appspot.com/6902070/diff/2001/state/api/serve.go#newcode48
state/api/serve.go:48: srv.tomb.Kill(srv.run(lis))
nice

https://codereview.appspot.com/6902070/diff/2001/state/api/serve.go#newcode53
state/api/serve.go:53: // Stop stops the server and returns when all requests
that
s/when../once all outstanding requests have been completed ?

https://codereview.appspot.com/6902070/diff/2001/state/api/serve.go#newcode65
state/api/serve.go:65: srv.wg.Done()
defer this line ? lis.Close() may panic, although unlikely

[fwereade, 2012-12-11 09:16:32]

LGTM assuming dave's suggestions

https://codereview.appspot.com/6902070/diff/2001/state/api/serve.go
File state/api/serve.go (right):

https://codereview.appspot.com/6902070/diff/2001/state/api/serve.go#newcode53
state/api/serve.go:53: // Stop stops the server and returns when all requests
that
On 2012/12/10 21:53:06, dfc wrote:
> s/when../once all outstanding requests have been completed ?
+1

https://codereview.appspot.com/6902070/diff/2001/state/api/serve.go#newcode65
state/api/serve.go:65: srv.wg.Done()
On 2012/12/10 21:53:06, dfc wrote:
> defer this line ? lis.Close() may panic, although unlikely

+1 also

[niemeyer, 2012-12-13 16:05:54]

https://codereview.appspot.com/6902070/diff/2001/state/api/serve.go
File state/api/serve.go (right):

https://codereview.appspot.com/6902070/diff/2001/state/api/serve.go#newcode69
state/api/serve.go:69: defer srv.wg.Done()
Why do we have two Adds and two Dones for a single call? The logic below doesn't
seem to justify that.

https://codereview.appspot.com/6902070/diff/2001/state/api/serve.go#newcode86
state/api/serve.go:86: <-srv.tomb.Dying()
Won't this block every single goroutine per connection made until the server is
closed?

[niemeyer, 2012-12-13 17:06:57]

niemeyer> rogpeppe2: https://codereview.appspot.com/6902070/ is reviewed
<niemeyer> rogpeppe2: Either I'm on crack, or the logic there isn't entirely
sound
* rogpeppe2 is now known as rogpeppe
<rogpeppe> niemeyer: you may well be right. let me check.
<rogpeppe> niemeyer: the first add is because we can't do an add before the
websocket handler function is invoked
<rogpeppe> niemeyer: the websocket handler uses a different goroutine for each
call (like the http server)
<niemeyer> rogpeppe: Yeah, even then it doesn't seem entirely okay
<niemeyer> rogpeppe: There are four different goroutines there
<niemeyer> rogpeppe: With adds all around
<niemeyer> rogpeppe: and it's not clear why
<niemeyer> rogpeppe: There is also a channel blocking at the end of each handled
connection, which also doesn't sound okay
<rogpeppe> niemeyer: we need to wait until everything has cleaned up before
dying
<niemeyer> rogpeppe: We need to do a lot of things
<niemeyer> rogpeppe: But that doesnt' mean we need a new goroutine for each one
of those things
<rogpeppe> niemeyer: i'm not sure how to avoid the goroutines
<niemeyer> rogpeppe: I suggest starting from the principle that you don't need
them, rather than from the principle that you do need them
<rogpeppe> niemeyer: the only way to get the http listener to quit is to close
its Listener
<niemeyer> rogpeppe: Also note how run(lis) never returns an error, despite the
fact it has an error return.. which means the outside goroutine, doing a Kill,
never does it
<niemeyer> rogpeppe: There's surprisingly little logic for that ton of logic 
<rogpeppe> niemeyer: yes, run never returns an error, because the error that it
almost always gets is "use of closed listener"
<rogpeppe> niemeyer: (if we used the error from http.Serve, that is)
<niemeyer> rogpeppe: Sorry, I'm not sure I understand what you mean
<rogpeppe> niemeyer: what error could Server.run return?
<niemeyer> rogpeppe: Things that never return an error should not have an error
return, and the error return from things that never return an error should not
be used
<niemeyer> rogpeppe: Both of these are false
<niemeyer> In the branch
<rogpeppe> niemeyer: that's true. i should probably remove the error return from
run
<niemeyer> rogpeppe: Yes, that's a start
<niemeyer> rogpeppe: What about that blocking channel at the end of every single
handled connection?
<rogpeppe> niemeyer: hmm, yes, that does seem a bit crackful
<rogpeppe> niemeyer: i think it needs to be a select
<rogpeppe> niemeyer: and wait for the st.run() to finish as well as the dying
tomb
<niemeyer> rogpeppe: Why? Why do we have a goroutine being created within the
per-connection goroutine?
<rogpeppe> niemeyer: i'm not sure i can use any less goroutines though
<rogpeppe> niemeyer: because we need somewhere to wait for the dying tomb
<rogpeppe> niemeyer: so that we can kill the connection
<niemeyer> rogpeppe: One of the reasons why we have a tomb is precisely to not
shut the door on running logic
<rogpeppe> niemeyer: we're not shutting the door on running logic, we're waiting
for websocket.JSON.Receive to receive an EOF, and quit the run loop
<rogpeppe> niemeyer: that's the only way to terminate that run loop AFAIK
<niemeyer> rogpeppe: That's not what conn.Close does
<rogpeppe> niemeyer: what does it do?
<niemeyer> rogpeppe: It closes the connection, doesn't it?
<niemeyer> rogpeppe: Irrespective of whatever is going on within the goroutine
<rogpeppe> niemeyer: yes. hmm, that will stop replies being sent, yesah
<rogpeppe> niemeyer: there probably needs to be something so that we can tell
when the message receiver is blocked on a message receive.
<rogpeppe> niemeyer: (pity we can't half-close the connection)
* mramm has quit (Remote host closed the connection)
<rogpeppe> niemeyer: perhaps before it does a message receive, it could send the
connection to another goroutine which will close it if it sees Dying.
<rogpeppe> niemeyer: once it's got the message, it can ask for the connection
back again
<rogpeppe> niemeyer: it would still be the same number of goroutines, but the
receiver would be more in control.
<niemeyer> rogpeppe: The point isn't closing, I think.. the point is stopping
and returning.. Close can be deferred on spot
<niemeyer> func(conn ...) {
<niemeyer>     defer conn.Close()
<niemeyer> }
<rogpeppe> niemeyer: the loop in run is blocked on receive. someone calls
Server.Stop. how do we stop the run loop?
<niemeyer> rogpeppe: Exactly..
<niemeyer> rogpeppe: That's the issue we must solve: how to stop and return..
that decision must be taken with context, unlike what's being done right now
<rogpeppe> niemeyer: i think closing the conn is the only way to do that
<niemeyer> rogpeppe: Of course not.. that's taking a cheap route from outside to
knock logic we don't control down
<rogpeppe> niemeyer: but you're right that it shouldn't be done outside of run
<rogpeppe> niemeyer: ok, so how do we get the run loop to unblock, if we can't
close the conn?
<niemeyer> rogpeppe: "if we can't close the Conn" is a red-herring
<niemeyer> rogpeppe: Where would you close the Conn?  That's where you get the
loop to unblock
<niemeyer> rogpeppe: You don't want to close the Conn without context
<rogpeppe> niemeyer: we need to close the conn *somewhere* right?
<niemeyer> <niemeyer> rogpeppe: The point isn't closing, I think.. the point is
stopping and returning.. Close can be deferred on spot
<niemeyer> <niemeyer> func(conn ...) {
<niemeyer> <niemeyer>     defer conn.Close()
<niemeyer> rogpeppe: That's where you close the Conn
<rogpeppe> niemeyer: i don't understand that code without further context
<niemeyer> rogpeppe: handler := websocket.Handler(func(conn *websocket.Conn) {
defer conn.Close()
<niemeyer> rogpeppe: The connection must be closed when the handler is done
running
<niemeyer> rogpeppe: We don't close the connection to interrupt.. we interrupt
to close the connection
<rogpeppe> niemeyer: how can that work? the handler blocks receiving a message.
if it's blocked receiving a message, how can it know that the server has been
killed?
<niemeyer> rogpeppe: That's the right question
<niemeyer> rogpeppe: Answering it is what we must find out
<rogpeppe> niemeyer: we could receive messages in a separate goroutine
<niemeyer> rogpeppe: That's not solving the problem
<rogpeppe> niemeyer: (that's still the same number of goroutines though)
<rogpeppe> niemeyer: isn't it?
<niemeyer> rogpeppe: We can have 10 goroutines.. we still must find a way to
interrupt in the right spot
<rogpeppe> niemeyer: i mean we could read messages in a goroutine, and send them
on a channel.
<rogpeppe> niemeyer: then select on that channel and Dying.
<niemeyer> rogpeppe: It's not.. you're saying "we can move the problem to a
separate goroutine".. surely we can. The problem is still in the other
goroutine, though.
<rogpeppe> niemeyer: we can't do it with one goroutine. period.
<niemeyer> rogpeppe: That's an irrelevant invariant.. we should look for the
solution, rather than how many goroutines we need to put an untold solution in
place. 
<rogpeppe> niemeyer: this would work, but you'll probably think it's messy:
http://paste.ubuntu.com/1430008/
<niemeyer> rogpeppe: How does conn.SetDeadline work?
<rogpeppe> niemeyer: i think it hooks into net.TCPConn.SetDeadline, but i'll
check
<niemeyer> rogpeppe: It does.. but what happens when the deadline expires?
<rogpeppe> niemeyer: you get an error that satisfies IsTimeout (IsTemporary?)
AFAIR
* mramm (~mramm@99-9-114-206.lightspeed.livnmi.sbcglobal.net) has joined
#juju-dev
<niemeyer> rogpeppe: Sure, but the interesting question is what happens to the
connection itself and the data that was being received when that takes place
<rogpeppe> niemeyer: i *think* it may be left intact. but i don't think that
helps us.
<rogpeppe> niemeyer: if something is doing a ReadFull and it gets a temporary
error, it is likely to be fatal
* otubo has quit (Ping timeout: 245 seconds)
<niemeyer> rogpeppe: Yeah, it'd likely introduce that kind of weird issue indeed
<rogpeppe> niemeyer: something like this is fairly clean:
http://paste.ubuntu.com/1430030/
<rogpeppe> niemeyer: then the close can go in the function as you suggest
<niemeyer> rogpeppe: Yeah, that looks like a nice directoin
<niemeyer> rogpeppe: The error must be taken care of, etc, but looks nice
<rogpeppe> niemeyer: that's what i was thinking of when i said
<rogpeppe> [16:38:48] <rogpeppe> niemeyer: i mean we could read messages in a
goroutine, and send them on a channel.
There are a bunch of things here that we discussed online. I'll just copy the
logs from IRC due to lazyness.

<rogpeppe> [16:39:02] <rogpeppe> niemeyer: then select on that channel and
Dying.
<rogpeppe> niemeyer: i'm not sure what the best thing to do with the errors is
though
<rogpeppe> niemeyer: we don't want to take the whole server down because a
single client sends an bogus request
<niemeyer> rogpeppe: Indeed, I guess that's fine

[rog, 2012-12-13 17:16:22]

Please take a look.

https://codereview.appspot.com/6902070/diff/2001/state/api/serve.go
File state/api/serve.go (right):

https://codereview.appspot.com/6902070/diff/2001/state/api/serve.go#newcode65
state/api/serve.go:65: srv.wg.Done()
On 2012/12/11 09:16:33, fwereade wrote:
> On 2012/12/10 21:53:06, dfc wrote:
> > defer this line ? lis.Close() may panic, although unlikely
> 
> +1 also

lis.Close cannot panic (we've checked the listener is ok elsewhere), and if it
does, the whole system goes down regardless anyway, so the defer doesn't help at
all.

i prefer the simpler straight-line logic if that's ok.

[rog, 2012-12-13 17:31:25]

Please take a look.

[rog, 2012-12-13 17:35:18]

Please take a look.

[rog, 2012-12-13 17:41:05]

Please take a look.

[rog, 2012-12-13 18:06:49]

Please take a look.

[niemeyer, 2012-12-13 18:15:02]

LGTM with the following addressed

https://codereview.appspot.com/6902070/diff/18001/state/api/serve.go
File state/api/serve.go (right):

https://codereview.appspot.com/6902070/diff/18001/state/api/serve.go#newcode143
state/api/serve.go:143: func (st *srvState) readRequests(c chan<- rpcRequest) {
defer close(c)

https://codereview.appspot.com/6902070/diff/18001/state/api/serve.go#newcode144
state/api/serve.go:144: var req rpcRequest
d

https://codereview.appspot.com/6902070/diff/18001/state/api/serve.go#newcode146
state/api/serve.go:146: req = rpcRequest{} // avoid any potential cross-message
contamination.
var req rpcRequest

https://codereview.appspot.com/6902070/diff/18001/state/api/serve.go#newcode157
state/api/serve.go:157: close(c)
d

[rog, 2012-12-13 18:48:59]

*** Submitted:

api/state: implement Server.Stop

This enables us to tear down everything cleanly and restart
when mongodb goes down.

R=dfc, fwereade, niemeyer
CC=
https://codereview.appspot.com/6902070

https://codereview.appspot.com/6902070/diff/18001/state/api/serve.go
File state/api/serve.go (right):

https://codereview.appspot.com/6902070/diff/18001/state/api/serve.go#newcode143
state/api/serve.go:143: func (st *srvState) readRequests(c chan<- rpcRequest) {
On 2012/12/13 18:15:03, niemeyer wrote:
> defer close(c)

Done.

https://codereview.appspot.com/6902070/diff/18001/state/api/serve.go#newcode144
state/api/serve.go:144: var req rpcRequest
On 2012/12/13 18:15:03, niemeyer wrote:
> d

left as-is, as discussed online

https://codereview.appspot.com/6902070/diff/18001/state/api/serve.go#newcode146
state/api/serve.go:146: req = rpcRequest{} // avoid any potential cross-message
contamination.
On 2012/12/13 18:15:03, niemeyer wrote:
> var req rpcRequest

ditto

https://codereview.appspot.com/6902070/diff/18001/state/api/serve.go#newcode157
state/api/serve.go:157: close(c)
On 2012/12/13 18:15:03, niemeyer wrote:
> d

Done.