Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net/rpc/jsonrpc: Missing support for JSON-RPC 2.0 #10929

Closed
pengux opened this issue May 21, 2015 · 20 comments
Closed

net/rpc/jsonrpc: Missing support for JSON-RPC 2.0 #10929

pengux opened this issue May 21, 2015 · 20 comments
Milestone

Comments

@pengux
Copy link
Contributor

@pengux pengux commented May 21, 2015

@minux minux added this to the Unplanned milestone May 21, 2015
@powerman
Copy link

@powerman powerman commented Aug 10, 2015

I'd like to implement it. I've enough experience with JSON RPC 2.0 (I'm author of several related Perl modules like JSON::RPC2 and Mojolicious::Plugin::JSONRPC2) but this task is going to be my first Go project (and it feels a bit too complicated for the first project).

I've checked current interface of net/rpc, and it looks like to support some features of JSON RPC 2.0 we'll need some… hacks. For example, right now I see only way to implement support for "batch" requests:

  • jsonrpc2 package should register own RPC handler "JSONRPC2.Batch"
  • when ReadRequestHeader() sees batch request it should set r.ServiceMethod = "JSONRPC2.Batch"; r.Seq=someFakeID and provide contents of batch request (slice of RPCs to call or just *json.RawMessage) as param for that handler
  • when "JSONRPC2.Batch" will be called it should pretend all RPC in that batch comes from another connection, i.e. start new ServeCodec(), write to it all RPC in that batch as separate requests, read from it replies to these RPC, stop ServeCodec() and return joined replies as it's own result
    • (this may introduce incompatibility with spec in case someone manage to send batch request inside another batch request - proposed implementation will execute it while spec doesn't allow this AFAIR)
  • send data returned by "JSONRPC2.Batch" as reply to original batch request

If there are simpler ways to do this - let me know, please.

@powerman
Copy link

@powerman powerman commented Aug 10, 2015

Another issue, which I don't think can be worked around without slight change of net/rpc interface, is support for custom error codes. Spec define error codes, some should be used by RPC engine implementation, others can be returned from user's RPC handler. If net/rpc had use Error error in Response struct instead of Error string then it would be possible for codecs to analyse dynamic type of returned error and decide what to do - if codec doesn't support anything than returning error as a string then it can use Error() to get it just before encoding, if codec/protocol does support extra data like error codes then it might get it from error of known dynamic type and use default error code otherwise.

The Response struct is exported, but doc states it's internal and serve only documentation purposes. Please correct me if I'm wrong, but technically changing type of Error field in that struct will break Go 1 Compatibility Guarantee, and this isn't an option? 😞

Only way around I see is serializing both error code and message into returned error's text, and then in codec try to analyse error text and detect is it contain only error message or both error code and message serialized that way. It's ugly, but should work.

@pengux
Copy link
Contributor Author

@pengux pengux commented Aug 11, 2015

I have resorted to not using the net/rpc because of the same reasons with custom error codes and batching. Instead, I added the RPC code inside the jsonrpc2 package. I'll open source that package in a few days here on Github so you can take a look at it.

@powerman
Copy link

@powerman powerman commented Aug 22, 2015

I've implemented jsonrpc2 codec for net/rpc: https://github.com/powerman/rpc-codec
This is my first non-helloworld Go project, so code review and any critique will be highly appreciated.

  • Server is full-featured. 😎
  • Client lack batch support (and I'm not sure is it worth to implement batch support for client at all - it looks too complicated and I'm not sure is it will be useful).
  • Code is based on net/rpc/jsonrpc, most of key changes implemented by single commit and can be easily reviewed.
  • There are a lot of new tests added.
  • It wasn't as simple as I like it to be because of mentioned above needs to work around net/rpc limitations. Also I suppose it may be a bit slower because of unmarshalling json more than once (needed to validate and conform to protocol). 😞
  • In TODO list:
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Aug 22, 2015

Is there any reason this needs to be in the standard library?

(See http://golang.org/doc/faq#x_in_std for background on the question.)

@powerman
Copy link

@powerman powerman commented Aug 22, 2015

I don't think it should be in the standard library. But when people see JSON-RPC 1.0 support in standard library, and "codecs" support in net/rpc they start looking for 2.0 support for net/rpc… and don't find any. So I think it's worth mentioning in this issue.

I decided to implement it as a codec instead of separate rpc-package mostly as educational task - to learn how net/rpc implemented, check is interfaces mentioned in JSON-RPC: a tale of interfaces are flexible enough, and to avoid implementing both protocol and RPC engine in my first project.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Aug 22, 2015

Perhaps we can just update the net/rpc/jsonrpc package doc to link say:

For JSON-RPC 2.0 support, see https://godoc.org/?q=jsonrpc+2.0

Thoughts?

@powerman
Copy link

@powerman powerman commented Aug 22, 2015

https://godoc.org/?q=json-rpc+2.0 will works better. 😄

@pengux
Copy link
Contributor Author

@pengux pengux commented Aug 22, 2015

It's good enough for me as I don't think JSONRPC2 can be fully supported in an idomatic way without rewriting the net/rpc package.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Aug 22, 2015

Want to send in a change updating the package doc?

@pengux
Copy link
Contributor Author

@pengux pengux commented Aug 25, 2015

@bradfitz Yes, I'm happy to do that, but do I need to follow the contributor process for it?

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Aug 25, 2015

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Dec 17, 2015

Ping!

@Dynom
Copy link

@Dynom Dynom commented Feb 4, 2016

I'm curious what the status of this issue is @pengux / @powerman ?

I see you both have done some work on the V2 codec, but there is nothing in the official pkg yet.

@powerman
Copy link

@powerman powerman commented Feb 5, 2016

@Dynom https://godoc.org/github.com/powerman/rpc-codec/jsonrpc2 works ok in several production services for about 6 months.

I've plans to add more usage examples in doc and probably option to switch it to "loose" mode (less sanity checks just like in net/rpc/jsonrpc to get higher speed).

Also I'm considering developing separate module for JSON RPC 2.0 (independent from net/rpc) - this should result in simpler implementation because there will be no needs to work around net/rpc limitations and more features because of same reason (like ability to provide transport-level details - for example client's IP - to called method).

@Dynom
Copy link

@Dynom Dynom commented Feb 8, 2016

Excellent work @powerman, I've currently worked around it by simply performing an HTTP request (it was a very trivial and static request to a JSON-RPC 2 server over HTTP). However I need to build several servers and clients that consume JSON-RPC 2 over HTTP, so your work is greatly appreciated.

I'll check it out and I'll contribute where I can.

@dwlnetnl
Copy link

@dwlnetnl dwlnetnl commented Feb 13, 2016

I've implemented a JSON-RPC 2.0 HTTP handler that also supports batch requests.

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 3, 2017

CL https://golang.org/cl/36330 mentions this issue.

@gopherbot gopherbot closed this in 7751d56 Feb 3, 2017
@Dynom
Copy link

@Dynom Dynom commented Feb 15, 2017

@dwlnetnl Would you mind updating your last comment with the correct URL? The current one seems broken.

@dwlnetnl
Copy link

@dwlnetnl dwlnetnl commented Mar 7, 2017

@Dynom I haven't that code not around anymore, sorry.

@golang golang locked and limited conversation to collaborators Mar 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.