rpc: refactor

rpc: refactor

This is a significant refactoring. I'm afraid I couldn't
see how to do it in small steps.

Changes include:
- factoring out of the websocket-specific RPC logic
from state/apiserver into rpc/jsoncodec.

- as a result, the rpc tests are now testing the same
codec as used in state/apiserver.

- the shutdown logic is hopefully cleaner and clearer.

- and most importantly, rpc connections are now
fully symmetric - the server can now make requests
to the client.

https://code.launchpad.net/~rogpeppe/juju-core/300-rpc-refactor/+merge/163705

(do not edit description out of merge proposal)

[rog, 2013-05-15 12:39:34]

Please take a look.

[dimitern, 2013-05-15 16:27:26]

Nice, LGTM and a few thoughts.

https://codereview.appspot.com/9410043/diff/7001/rpc/jsoncodec/codec.go
File rpc/jsoncodec/codec.go (right):

https://codereview.appspot.com/9410043/diff/7001/rpc/jsoncodec/codec.go#newco...
rpc/jsoncodec/codec.go:72: c.mu.Unlock()
why not defer c.mu.Unlock() instead?

https://codereview.appspot.com/9410043/diff/7001/rpc/jsoncodec/codec.go#newco...
rpc/jsoncodec/codec.go:89: log.Debugf("rpc/wsjson: <- %s", m)
s/wsjson/jsoncodec/ ? and below

https://codereview.appspot.com/9410043/diff/7001/rpc/jsoncodec/codec.go#newco...
rpc/jsoncodec/codec.go:151: log.Debugf("api: -> marshal error: %v", err)
should this be rpc/jsoncodec: or rpc/wsjson: or something?

https://codereview.appspot.com/9410043/diff/7001/rpc/jsoncodec/codec.go#newco...
rpc/jsoncodec/codec.go:154: log.Debugf("api: -> %s", data)
ditto

https://codereview.appspot.com/9410043/diff/7001/rpc/jsoncodec/codec_test.go
File rpc/jsoncodec/codec_test.go (right):

https://codereview.appspot.com/9410043/diff/7001/rpc/jsoncodec/codec_test.go#...
rpc/jsoncodec/codec_test.go:14: type suite struct{}
codecSuite?

https://codereview.appspot.com/9410043/diff/7001/rpc/server.go
File rpc/server.go (right):

https://codereview.appspot.com/9410043/diff/7001/rpc/server.go#newcode112
rpc/server.go:112: dead chan struct{}
just a random idea - can we use tomb here instead?

https://codereview.appspot.com/9410043/diff/7001/rpc/server.go#newcode182
rpc/server.go:182: _, err := methods(rootValue.Type())
if _, err := ...; err != nil { ...

https://codereview.appspot.com/9410043/diff/7001/rpc/server.go#newcode232
rpc/server.go:232: _ = conn.codec.Close()
probably send it on the dead channel or some other ?

https://codereview.appspot.com/9410043/diff/7001/rpc/server.go#newcode294
rpc/server.go:294: panic("unreachable")
we should start getting rid of these, since we'll be using go1.1

[rog, 2013-05-15 17:15:25]

Please take a look.

https://codereview.appspot.com/9410043/diff/7001/rpc/jsoncodec/codec.go
File rpc/jsoncodec/codec.go (right):

https://codereview.appspot.com/9410043/diff/7001/rpc/jsoncodec/codec.go#newco...
rpc/jsoncodec/codec.go:72: c.mu.Unlock()
On 2013/05/15 16:27:26, dimitern wrote:
> why not defer c.mu.Unlock() instead?

because i'd prefer not to close the underlying connection within the scope of
the mutex. call me paranoid :-)

https://codereview.appspot.com/9410043/diff/7001/rpc/jsoncodec/codec.go#newco...
rpc/jsoncodec/codec.go:89: log.Debugf("rpc/wsjson: <- %s", m)
On 2013/05/15 16:27:26, dimitern wrote:
> s/wsjson/jsoncodec/ ? and below

Done.

https://codereview.appspot.com/9410043/diff/7001/rpc/jsoncodec/codec.go#newco...
rpc/jsoncodec/codec.go:151: log.Debugf("api: -> marshal error: %v", err)
On 2013/05/15 16:27:26, dimitern wrote:
> should this be rpc/jsoncodec: or rpc/wsjson: or something?

Done.

https://codereview.appspot.com/9410043/diff/7001/rpc/jsoncodec/codec.go#newco...
rpc/jsoncodec/codec.go:154: log.Debugf("api: -> %s", data)
On 2013/05/15 16:27:26, dimitern wrote:
> ditto

Done.

https://codereview.appspot.com/9410043/diff/7001/rpc/jsoncodec/codec_test.go
File rpc/jsoncodec/codec_test.go (right):

https://codereview.appspot.com/9410043/diff/7001/rpc/jsoncodec/codec_test.go#...
rpc/jsoncodec/codec_test.go:14: type suite struct{}
On 2013/05/15 16:27:26, dimitern wrote:
> codecSuite?

nah, there's only one suite here. if we add more, i'll change the name.

https://codereview.appspot.com/9410043/diff/7001/rpc/server.go
File rpc/server.go (right):

https://codereview.appspot.com/9410043/diff/7001/rpc/server.go#newcode112
rpc/server.go:112: dead chan struct{}
On 2013/05/15 16:27:26, dimitern wrote:
> just a random idea - can we use tomb here instead?

i started off using tomb, but it's more than we need - we don't have a dying
phase. the code is, i think, just as clear and short without it.

https://codereview.appspot.com/9410043/diff/7001/rpc/server.go#newcode182
rpc/server.go:182: _, err := methods(rootValue.Type())
On 2013/05/15 16:27:26, dimitern wrote:
> if _, err := ...; err != nil { ...

Done.

https://codereview.appspot.com/9410043/diff/7001/rpc/server.go#newcode232
rpc/server.go:232: _ = conn.codec.Close()
On 2013/05/15 16:27:26, dimitern wrote:
> probably send it on the dead channel or some other ?

we can't do that - we're *receiving* from the dead channel here.

the problem is that we've got two errors here - the error from the input loop
and the error from closing the codec. tbh i don't mind discarding the close
error - i can't think of a situation in which it would be very useful.

https://codereview.appspot.com/9410043/diff/7001/rpc/server.go#newcode294
rpc/server.go:294: panic("unreachable")
On 2013/05/15 16:27:26, dimitern wrote:
> we should start getting rid of these, since we'll be using go1.1

+1. let's do them all in one fell swoop though.

[fwereade, 2013-05-16 13:50:02]

Essentially LGTM, but I would appreciate a bit more verbosity in the variable
names -- I find it really does make a difference especially when first reading
the code. Comments follow.

https://codereview.appspot.com/9410043/diff/7001/rpc/server.go
File rpc/server.go (right):

https://codereview.appspot.com/9410043/diff/7001/rpc/server.go#newcode232
rpc/server.go:232: _ = conn.codec.Close()
On 2013/05/15 17:15:26, rog wrote:
> On 2013/05/15 16:27:26, dimitern wrote:
> > probably send it on the dead channel or some other ?
> 
> we can't do that - we're *receiving* from the dead channel here.
> 
> the problem is that we've got two errors here - the error from the input loop
> and the error from closing the codec. tbh i don't mind discarding the close
> error - i can't think of a situation in which it would be very useful.

At least log it with a warning, please.

https://codereview.appspot.com/9410043/diff/9002/rpc/jsoncodec/conn.go
File rpc/jsoncodec/conn.go (right):

https://codereview.appspot.com/9410043/diff/9002/rpc/jsoncodec/conn.go#newcode10
rpc/jsoncodec/conn.go:10: // NewWS returns an rpc codec that uses the given
websocket
NewWebsocket?

https://codereview.appspot.com/9410043/diff/9002/rpc/server.go
File rpc/server.go (left):

https://codereview.appspot.com/9410043/diff/9002/rpc/server.go#oldcode185
rpc/server.go:185: return o, a, nil
I'm not really loving these m, o, a vars...

https://codereview.appspot.com/9410043/diff/9002/rpc/server.go
File rpc/server.go (right):

https://codereview.appspot.com/9410043/diff/9002/rpc/server.go#newcode335
rpc/server.go:335: closing := conn.closing
can't we just unlock here?

https://codereview.appspot.com/9410043/diff/9002/rpc/server.go#newcode341
rpc/server.go:341: if closing {
...and use an else here for clarity? I misread it first time round.

[rog, 2013-05-16 17:33:50]

*** Submitted:

rpc: refactor

This is a significant refactoring. I'm afraid I couldn't
see how to do it in small steps.

Changes include:
- factoring out of the websocket-specific RPC logic
from state/apiserver into rpc/jsoncodec.

- as a result, the rpc tests are now testing the same
codec as used in state/apiserver.

- the shutdown logic is hopefully cleaner and clearer.

- and most importantly, rpc connections are now
fully symmetric - the server can now make requests
to the client.

R=dimitern, fwereade
CC=
https://codereview.appspot.com/9410043

https://codereview.appspot.com/9410043/diff/7001/rpc/server.go
File rpc/server.go (right):

https://codereview.appspot.com/9410043/diff/7001/rpc/server.go#newcode232
rpc/server.go:232: _ = conn.codec.Close()
On 2013/05/16 13:50:03, fwereade wrote:
> On 2013/05/15 17:15:26, rog wrote:
> > On 2013/05/15 16:27:26, dimitern wrote:
> > > probably send it on the dead channel or some other ?
> > 
> > we can't do that - we're *receiving* from the dead channel here.
> > 
> > the problem is that we've got two errors here - the error from the input
loop
> > and the error from closing the codec. tbh i don't mind discarding the close
> > error - i can't think of a situation in which it would be very useful.
> 
> At least log it with a warning, please.

Done.

https://codereview.appspot.com/9410043/diff/9002/rpc/server.go
File rpc/server.go (left):

https://codereview.appspot.com/9410043/diff/9002/rpc/server.go#oldcode185
rpc/server.go:185: return o, a, nil
On 2013/05/16 13:50:03, fwereade wrote:
> I'm not really loving these m, o, a vars...

Done.
(i think)

https://codereview.appspot.com/9410043/diff/9002/rpc/server.go
File rpc/server.go (right):

https://codereview.appspot.com/9410043/diff/9002/rpc/server.go#newcode335
rpc/server.go:335: closing := conn.closing
On 2013/05/16 13:50:03, fwereade wrote:
> can't we just unlock here?

no - it's important that we cannot start a new request once closing has been set
(otherwise we can start servicing a new incoming request after we've called
Kill, which might mean that we can't shut down the server).

i'll add a comment.

https://codereview.appspot.com/9410043/diff/9002/rpc/server.go#newcode341
rpc/server.go:341: if closing {
On 2013/05/16 13:50:03, fwereade wrote:
> ...and use an else here for clarity? I misread it first time round.

i'm afraid not.