sourcegraph / jsonrpc2

Package jsonrpc2 provides a client and server implementation of JSON-RPC 2.0 (http://www.jsonrpc.org/specification)
MIT License
196 stars 62 forks source link

Add pluggable transport interface + WebSocket support #4

Closed sqs closed 7 years ago

sqs commented 7 years ago

TODOs:

keegancsmith commented 7 years ago

So when you read the jsonrpc2 spec, it doesn't dictate anything about the transport. So I think receiving an object and sending an object as the interface make a lot of sense. As such I think we can probably simplify our Transport interface to just dealing with []bytes (ie get rid of returning a Reader). As it is, the underlying implementations has the content-length, so our underlying Transport can nicely create correctly sized []byte slices.

It may even make sense for Transport to just be:

type Transport struct {
  ReadJSON(v interface{}) error
  WriteJSON(v interface{}) error
}

that would match up with the interface websocket exposes, and allows the transport re-use buffers. The more I think about it, I think that is the best interface for Transport.

As for the current Codec interface, you are right it is only tied to the stream transport. I wonder if we should bother exporting it at all? What do you think about NewStreamTransport taking an enum specifying the underlying codec to use. Or creating a NewVSCodeTransport, and adding other functions as we support them? Right now vscode stream transport is the only streaming implementation I am aware of. Also creating a new stream transport from scratch is pretty trivial.

I also found this in the websocket docs:

Connections support one concurrent reader and one concurrent writer.

I think we satisfy this, but just double checking with you if this is true.

sqs commented 7 years ago

@keegancsmith Thanks for the review.

(re: making the transport take interface{})

Makes total sense to me. Good call.

As for the current Codec interface, you are right it is only tied to the stream transport. I wonder if we should bother exporting it at all? What do you think about NewStreamTransport taking an enum specifying the underlying codec to use. Or creating a NewVSCodeTransport, and adding other functions as we support them?

I agree that it'd suffice for our strict current needs to couple them. But I think keeping them separate makes it easier for other people to use the library, and for us to use the library when we don't want the overhead of those header lines. I will keep it as-is for now.

Connections support one concurrent reader and one concurrent writer. I think we satisfy this, but just double checking with you if this is true.

Yep.

benchmarks on the codecs would be useful, especially from a memory perspective.

Will do.

sqs commented 7 years ago

After thinking about it more, you are right. I think we can remove the transport and just make the thing that flushes net.Conns into something that wraps the underlying "codec" and flushes the conn after writes. Will look into this.

sqs commented 7 years ago

I was not able to remove the distinction between the ObjectStream and the codec without compromising elsewhere. But I did simplify it a LOT by using interface{} and not trying to be clever with readers, per your comments. And the name "transport" did not sit well with me, so I renamed it from Transport to ObjectStream.

I'm going to merge. Thanks!

sqs commented 7 years ago

squashed on CLI + merged to master