code review 2696041: rpc: expose Server type to allow multiple RPC Server in...

rpc: expose Server type to allow multiple RPC Server instances

[adg, 2010-10-24 01:21:42]

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

I'd like you to review this change.

[r2, 2010-10-24 03:15:45]

surely there are tests too?

-rob

[adg, 2010-10-24 03:24:06]

I'd like to get clearance on the changes before going to the trouble
of testing them.

The existing tests test this indirectly by testing the DefaultServer,
so it'll just be adding a level of indirection to them.

On 24 October 2010 14:15, Rob 'Commander' Pike <r@google.com> wrote:
> surely there are tests too?
>
> -rob
>
>

[rsc1, 2010-10-24 03:47:23]

Seems reasonable.  You're going to have to figure out
how to let clients connect to a different path too, 
I guess.

Also, please think about whether we can give the server
a method that takes the request as a parameter and 
returns/fills in a response, so that people who want
to use a transport that doesn't work well as a 
ServerCodec can still do that.  Mike might have
some comments about that.  [+msolo]

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

http://codereview.appspot.com/2696041/diff/2001/src/pkg/rpc/server.go#newcode189
src/pkg/rpc/server.go:189: //	- public method
s/public/exported/

http://codereview.appspot.com/2696041/diff/2001/src/pkg/rpc/server.go#newcode191
src/pkg/rpc/server.go:191: //	- one return value of type os.Error
s/value/value,/

http://codereview.appspot.com/2696041/diff/2001/src/pkg/rpc/server.go#newcode192
src/pkg/rpc/server.go:192: // It returns an error if the receiver is not public
or has no
s/public/an exported type/.
I don't remember why that matters, actually.

http://codereview.appspot.com/2696041/diff/2001/src/pkg/rpc/server.go#newcode207
src/pkg/rpc/server.go:207: if s.typ.PkgPath() != "" && !isPublic(sname) {
i know it says public here but let's fix all these
to say exported.  that's the word in the spec, and
it has the side benefit of not being the word in C++ or Java.

http://codereview.appspot.com/2696041/diff/2001/src/pkg/rpc/server.go#newcode322
src/pkg/rpc/server.go:322: // A ServerCodec implements reading of RPC requests
and writing of
seems like a gratuitous move, but okay

http://codereview.appspot.com/2696041/diff/2001/src/pkg/rpc/server.go#newcode472
src/pkg/rpc/server.go:472: // ServeHTTP implements an http.Handler that answers
RPC requests.
Probably want to export rpcPath too?
And debugPath?

[adg, 2010-10-25 04:34:13]

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

I'd like you to review this change.

[msolo, 2010-10-25 20:36:41]

In general, I feel this is on the right path.

I wouldn't be surprised if something had to be added to interact externally with
the serviceMap at some point.

Removing the path from the CONNECT error is probably fine, though again, I
wouldn't be surprised if one needed to end up customizing/accessing the
connected var.

On 2010/10/25 04:34:13, adg wrote:
> Hello r, rsc, msolo (cc:
http://mail.google.com/mail/?view=cm&fs=1&tf=1&to=golang-dev@googlegroups.com...),
> 
> I'd like you to review this change.

[sougou, 2010-10-25 21:13:17]

I had sent this email to rsc earlier. Do you think you'll be able to break up
the input() function such that it can synchronously serve a single request?

I'm currently working with msolo on a server project. I've been trying to write
a HTTP POST based handler for jsonrpc, and possibly other protocols too.
Basically, we want to send each RPC request as an HTTP POST where the contents
of the request are in the Body.
The way I tried to do this is by creating a proxy object that masquerades as a
connection that the jsonrpc codec expects:

// JsonHttpBroker implements the ReadWriteCloser interface for the Json HTTP
server.
type JsonHttpBroker struct {
  io.Writer           // Initialize with http.ResponseWriter
  io.ReadCloser   // Initialize with http.Request's Body member
}
You can find the code in /home/sougou/jsonhttprpc.go

But this code doesn't work. This is what we think the reason is:
1. ServeJsonHttpRpc (HTTP handler) eventually ends up calling input() in
rpc/server.go
2. The input function receives the request correctly from the http.Request's
Body member.
3. It launches the go routine to execute the rpc call.
4. It loops back to read the next request, and receives an EOF from the Body.
5. It exits the loop, closes the connection, and returns to ServeJsonHttpRpc.
6. ServeJsonHttpRpc returns back to the http library, which ends the request.

However, all this happens before the go routine (step 3) finishes writing the
data on the ResponseWriter. This results in the client receiving an empty
response.
In other words, we need some way/mechanism to wait for step 3 to finish before
ServeJsonHttpRpc can return back to the caller.

Some of the options we've brainstormed, in order of decreasing excitement:
1. Pull out parts of the input() function to make them individually reusable.
We'll basically need something we can call that will process one request without
launching a go routine. So, the function will return only after the rpc call has
completed.
2. Add some synchronization mechanism that can be used to wait for the go
routine to complete, maybe a channel that ServeJsonHttpRpc can wait on.
3. Make the function map public, so ServeJsonHttpRpc can do its own version of
input.

On 2010/10/25 20:36:41, msolo wrote:
> In general, I feel this is on the right path.
> 
> I wouldn't be surprised if something had to be added to interact externally
with
> the serviceMap at some point.
> 
> Removing the path from the CONNECT error is probably fine, though again, I
> wouldn't be surprised if one needed to end up customizing/accessing the
> connected var.
> 
> On 2010/10/25 04:34:13, adg wrote:
> > Hello r, rsc, msolo (cc:
>
http://mail.google.com/mail/?view=cm&fs=1&tf=1&to=golang-dev%40googlegroups.c...),
> > 
> > I'd like you to review this change.

[r, 2010-10-25 23:43:25]

time for tests

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

http://codereview.appspot.com/2696041/diff/14001/src/pkg/rpc/server.go#newcod...
src/pkg/rpc/server.go:175: type Server struct {
doc comment please

http://codereview.appspot.com/2696041/diff/14001/src/pkg/rpc/server.go#newcod...
src/pkg/rpc/server.go:180: func NewServer() *Server {
doc comment please

[adg, 2010-10-25 23:54:58]

On 26 October 2010 08:13,  <sougou@google.com> wrote:
> I had sent this email to rsc earlier. Do you think you'll be able to
> break up the input() function such that it can synchronously serve a
> single request?
>
> I'm currently working with msolo on a server project. I've been trying
> to write a HTTP POST based handler for jsonrpc, and possibly other
> protocols too.
> Basically, we want to send each RPC request as an HTTP POST where the
> contents of the request are in the Body.
> The way I tried to do this is by creating a proxy object that
> masquerades as a connection that the jsonrpc codec expects:
>
> // JsonHttpBroker implements the ReadWriteCloser interface for the Json
> HTTP server.
> type JsonHttpBroker struct {
>  io.Writer           // Initialize with http.ResponseWriter
>  io.ReadCloser   // Initialize with http.Request's Body member
> }
> You can find the code in /home/sougou/jsonhttprpc.go
>
> But this code doesn't work. This is what we think the reason is:
> 1. ServeJsonHttpRpc (HTTP handler) eventually ends up calling input() in
> rpc/server.go
> 2. The input function receives the request correctly from the
> http.Request's Body member.
> 3. It launches the go routine to execute the rpc call.
> 4. It loops back to read the next request, and receives an EOF from the
> Body.
> 5. It exits the loop, closes the connection, and returns to
> ServeJsonHttpRpc.
> 6. ServeJsonHttpRpc returns back to the http library, which ends the
> request.
>
> However, all this happens before the go routine (step 3) finishes
> writing the data on the ResponseWriter. This results in the client
> receiving an empty response.
> In other words, we need some way/mechanism to wait for step 3 to finish
> before ServeJsonHttpRpc can return back to the caller.

I just took a crack at separating (what is now called) ServeCodec into
two functions. ServeCodec calls a new method ReadRequest to read in
the request, and then makes the service.call() in a new goroutine.
ReadRequest returns a *service, argv, replyv, req, and error state. It
gives you all you need to make the service.call(), and that would suit
your "one-shot" approach.

This approach requires exporting the service and methodType types, and
possibly more. Things then start to get more complicated than I'm
comfortable with (I'm already complicating rpc more with this CL). I
think with some more thought and some judicious refactoring, the
problem could be split a little neater.

> Some of the options we've brainstormed, in order of decreasing
> excitement:
> 1. Pull out parts of the input() function to make them individually
> reusable. We'll basically need something we can call that will process
> one request without launching a go routine. So, the function will return
> only after the rpc call has completed.

This is what I tried, and I think this would be the way to go if we
were to make further changes to rpc.

> 2. Add some synchronization mechanism that can be used to wait for the
> go routine to complete, maybe a channel that ServeJsonHttpRpc can wait
> on.
> 3. Make the function map public, so ServeJsonHttpRpc can do its own
> version of input.

I haven't thought much about the implications of these, but they're
not very appealing from the outset.

Andrew

[adg, 2010-10-26 02:52:45]

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

Please take another look.

[r, 2010-10-27 23:08:05]

LGTM

[adg, 2010-10-28 00:06:21]

*** Submitted as http://code.google.com/p/go/source/detail?r=d069cce810cf ***

rpc: expose Server type to allow multiple RPC Server instances

R=r, rsc, msolo, sougou
CC=golang-dev
http://codereview.appspot.com/2696041

Committer: Andrew Gerrand <adg@golang.org>

[msolo, 2010-10-28 01:11:05]

This is good. I think there still might be a need to break out the
meat of the ServeCodec for-loop to allow for other enveloping
techniques - like just using HTTP itself.

We'll know more once we experiment and try to integrate our existing code.

On Wed, Oct 27, 2010 at 5:06 PM,  <adg@golang.org> wrote:
> *** Submitted as
> http://code.google.com/p/go/source/detail?r=d069cce810cf ***
>
> rpc: expose Server type to allow multiple RPC Server instances
>
> R=r, rsc, msolo, sougou
> CC=golang-dev
> http://codereview.appspot.com/2696041
>
> Committer: Andrew Gerrand <adg@golang.org>
>
>
> http://codereview.appspot.com/2696041/
>