code review 4080058: rpc: make more tolerant of errors.

rpc: make more tolerant of errors.
Add Error type to enable clients to distinguish
between local and remote errors.
Also return "connection shut down error" after
the first error return rather than returning the
same error each time.

[rog, 2011-02-07 17:07:01]

Hello r (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[rog, 2011-02-07 18:18:31]

Hello r (cc: golang-dev@googlegroups.com),

Please take another look.

[r, 2011-02-07 18:29:37]

generally good, although i see lots of things in the original code i'd like to
improve

http://codereview.appspot.com/4080058/diff/1006/src/pkg/rpc/client.go
File src/pkg/rpc/client.go (right):

http://codereview.appspot.com/4080058/diff/1006/src/pkg/rpc/client.go#newcode26
src/pkg/rpc/client.go:26: const errShutdown = os.ErrorString("connection is shut
down")
does it make sense to export this name?

http://codereview.appspot.com/4080058/diff/1006/src/pkg/rpc/client.go#newcode228
src/pkg/rpc/client.go:228: return os.ErrorString("rpc: already closed")
now would be a good time to make this an Error too.

http://codereview.appspot.com/4080058/diff/1006/src/pkg/rpc/server.go
File src/pkg/rpc/server.go (right):

http://codereview.appspot.com/4080058/diff/1006/src/pkg/rpc/server.go#newcode429
src/pkg/rpc/server.go:429: func (server *Server) readHeader(codec ServerCodec)
(*Request, *service, *methodType, os.Error) {
tthis code would benefit by naming the return values and using those variables.
the return statements are reminiscent of Fortran calls.

http://codereview.appspot.com/4080058/diff/1006/src/pkg/rpc/server.go#newcode429
src/pkg/rpc/server.go:429: func (server *Server) readHeader(codec ServerCodec)
(*Request, *service, *methodType, os.Error) {
isn't this really readRequest?

[rog, 2011-02-07 18:49:30]

> generally good, although i see lots of things in the original code i'd
> like to improve

yes, me too, but CLs can't overlap.

http://codereview.appspot.com/4080058/diff/1006/src/pkg/rpc/client.go
File src/pkg/rpc/client.go (right):

http://codereview.appspot.com/4080058/diff/1006/src/pkg/rpc/client.go#newcode26
src/pkg/rpc/client.go:26: const errShutdown = os.ErrorString("connection is shut
down")
On 2011/02/07 18:29:37, r wrote:
> does it make sense to export this name?

Done.

http://codereview.appspot.com/4080058/diff/1006/src/pkg/rpc/client.go#newcode228
src/pkg/rpc/client.go:228: return os.ErrorString("rpc: already closed")
maybe ErrShutDown would be appropriate here too?

http://codereview.appspot.com/4080058/diff/1006/src/pkg/rpc/server.go
File src/pkg/rpc/server.go (right):

http://codereview.appspot.com/4080058/diff/1006/src/pkg/rpc/server.go#newcode429
src/pkg/rpc/server.go:429: func (server *Server) readHeader(codec ServerCodec)
(*Request, *service, *methodType, os.Error) {
On 2011/02/07 18:29:37, r wrote:
> isn't this really readRequest?

well, it only reads the request header, but i see your point.

http://codereview.appspot.com/4080058/diff/1006/src/pkg/rpc/server.go#newcode429
src/pkg/rpc/server.go:429: func (server *Server) readHeader(codec ServerCodec)
(*Request, *service, *methodType, os.Error) {
On 2011/02/07 18:29:37, r wrote:
> tthis code would benefit by naming the return values and using those
variables.
> the return statements are reminiscent of Fortran calls.

Done.

[r, 2011-02-07 19:04:28]

http://codereview.appspot.com/4080058/diff/5005/src/pkg/rpc/server.go
File src/pkg/rpc/server.go (right):

http://codereview.appspot.com/4080058/diff/5005/src/pkg/rpc/server.go#newcode449
src/pkg/rpc/server.go:449: service, ok = server.serviceMap[serviceMethod[0]]
drop the ok and check service == nil

http://codereview.appspot.com/4080058/diff/5005/src/pkg/rpc/server.go#newcode455
src/pkg/rpc/server.go:455: mtype, ok = service.method[serviceMethod[1]]
ditto

[rog, 2011-02-07 19:13:06]

i wonder if gob and rpc should implement a Temporary
method on their error types to enable distinguishing between
recoverable and unrecoverable decoding errors.

the loop in Server.ServeCodec will currently loop forever
if the underlying reader continually returns some error
other than EOF or UnexpectedEOF.


On 7 February 2011 18:29,  <r@golang.org> wrote:
> generally good, although i see lots of things in the original code i'd
> like to improve
>
>
>
> http://codereview.appspot.com/4080058/diff/1006/src/pkg/rpc/client.go
> File src/pkg/rpc/client.go (right):
>
>
http://codereview.appspot.com/4080058/diff/1006/src/pkg/rpc/client.go#newcode26
> src/pkg/rpc/client.go:26: const errShutdown = os.ErrorString("connection
> is shut down")
> does it make sense to export this name?
>
>
http://codereview.appspot.com/4080058/diff/1006/src/pkg/rpc/client.go#newcode228
> src/pkg/rpc/client.go:228: return os.ErrorString("rpc: already closed")
> now would be a good time to make this an Error too.
>
> http://codereview.appspot.com/4080058/diff/1006/src/pkg/rpc/server.go
> File src/pkg/rpc/server.go (right):
>
>
http://codereview.appspot.com/4080058/diff/1006/src/pkg/rpc/server.go#newcode429
> src/pkg/rpc/server.go:429: func (server *Server) readHeader(codec
> ServerCodec) (*Request, *service, *methodType, os.Error) {
> tthis code would benefit by naming the return values and using those
> variables. the return statements are reminiscent of Fortran calls.
>
>
http://codereview.appspot.com/4080058/diff/1006/src/pkg/rpc/server.go#newcode429
> src/pkg/rpc/server.go:429: func (server *Server) readHeader(codec
> ServerCodec) (*Request, *service, *methodType, os.Error) {
> isn't this really readRequest?
>
> http://codereview.appspot.com/4080058/
>

[rog, 2011-02-07 19:17:11]

PTAL.
(slight change made at beginning of ServeCodec loop too)

On 7 February 2011 19:04,  <r@golang.org> wrote:
>
> http://codereview.appspot.com/4080058/diff/5005/src/pkg/rpc/server.go
> File src/pkg/rpc/server.go (right):
>
>
http://codereview.appspot.com/4080058/diff/5005/src/pkg/rpc/server.go#newcode449
> src/pkg/rpc/server.go:449: service, ok =
> server.serviceMap[serviceMethod[0]]
> drop the ok and check service == nil
>
>
http://codereview.appspot.com/4080058/diff/5005/src/pkg/rpc/server.go#newcode455
> src/pkg/rpc/server.go:455: mtype, ok = service.method[serviceMethod[1]]
> ditto
>
> http://codereview.appspot.com/4080058/
>

[rog, 2011-02-07 19:40:01]

Hello r (cc: golang-dev@googlegroups.com),

Please take another look.

[r, 2011-02-07 20:34:25]

one small but important detail i should have caught before

http://codereview.appspot.com/4080058/diff/17001/src/pkg/rpc/client.go
File src/pkg/rpc/client.go (right):

http://codereview.appspot.com/4080058/diff/17001/src/pkg/rpc/client.go#newcode18
src/pkg/rpc/client.go:18: // Error represents an error that has been returned
from
apologies but i think this is the wrong name.  it should be called ServerError
or something like that.  think of how it reads from the client code trying to
use its type to glean information.

[rog, 2011-02-08 13:27:29]

PTAL

http://codereview.appspot.com/4080058/diff/17001/src/pkg/rpc/client.go
File src/pkg/rpc/client.go (right):

http://codereview.appspot.com/4080058/diff/17001/src/pkg/rpc/client.go#newcode18
src/pkg/rpc/client.go:18: // Error represents an error that has been returned
from
On 2011/02/07 20:34:25, r wrote:
> apologies but i think this is the wrong name.  it should be called ServerError
> or something like that.  think of how it reads from the client code trying to
> use its type to glean information.

Done.

[r, 2011-02-08 18:40:34]

http://codereview.appspot.com/4080058/diff/19001/src/pkg/rpc/client.go
File src/pkg/rpc/client.go (right):

http://codereview.appspot.com/4080058/diff/19001/src/pkg/rpc/client.go#newcode18
src/pkg/rpc/client.go:18: // Error represents an error that has been returned
from
ServerError

http://codereview.appspot.com/4080058/diff/19001/src/pkg/rpc/client.go#newcode26
src/pkg/rpc/client.go:26: const ErrShutdown = os.ErrorString("connection is shut
down")
should this be a ServerError? i can see arguments either way.

http://codereview.appspot.com/4080058/diff/19001/src/pkg/rpc/server_test.go
File src/pkg/rpc/server_test.go (right):

http://codereview.appspot.com/4080058/diff/19001/src/pkg/rpc/server_test.go#n...
src/pkg/rpc/server_test.go:137: // Non-existent method
s/-//

[rog, 2011-02-09 11:52:42]

http://codereview.appspot.com/4080058/diff/19001/src/pkg/rpc/client.go
File src/pkg/rpc/client.go (right):

http://codereview.appspot.com/4080058/diff/19001/src/pkg/rpc/client.go#newcode18
src/pkg/rpc/client.go:18: // Error represents an error that has been returned
from
On 2011/02/08 18:40:35, r wrote:
> ServerError

Done.

http://codereview.appspot.com/4080058/diff/19001/src/pkg/rpc/client.go#newcode26
src/pkg/rpc/client.go:26: const ErrShutdown = os.ErrorString("connection is shut
down")
On 2011/02/08 18:40:35, r wrote:
> should this be a ServerError? i can see arguments either way.

I don't think so. My intention was to make it possible for an RPC client to
distinguish between a call error (for instance a invalid argument type or other
call-specific error) and a fatal error which would necessite a re-dial of the
server to continue.

[r, 2011-02-09 18:51:10]

LGTM

[rog, 2011-02-18 11:27:48]

*** Abandoned ***

[rog, 2011-02-18 11:28:59]

strange that this wasn't marked as submitted, even though it was.

On 18 February 2011 11:27,  <rogpeppe@gmail.com> wrote:
> *** Abandoned ***
>
> http://codereview.appspot.com/4080058/
>