jsonrpc2: Add batch and notify support by shazow · Pull Request #49 · vipnode/vipnode

shazow · 2019-07-05T20:41:18Z

This should get us very close to #43. The goal is to have a complete JSONRPC2 implementation with a permissive license that can be usable for all functions of Ethereum nodes. (Aside: Might even be worth pulling it out into a standalone library at that point, to allow for separate versioning?)

Add support for batched messages. (Closes jsonrpc2: Add batch request support #42)
- Read: Batched messages are unbatched in the consumer API, so the consumer can pretend they're individual messages.
- Write: Introduced a new Replier interface, which the internal API uses to reply to messages. When a message is part of a batch, replying to it will buffer it into a batch reply internally until the correct number of replies are accumulated, then they're flushed as a batch.
Add support for notifications (messages without IDs, ignored in batch replies), callable via the new Notify interface. (Closes jsonrpc2: Add notification support #41)

Fix pool metadata bugs

Needs more tests

…ecoder

Mildly breaks the public API (probably unused)

shazow · 2019-07-08T20:45:01Z

-		},
-		ID:      req.ID,
-		Version: Version,
+func (s *Server) Handle(ctx context.Context, req *Request) *Response {


This is breaking the public API, but nobody is using it yet and it's moderately internal, sooo seems safe? 🤷‍♂️

ryanschneider

Overall lgtm, minor questions/comments below.

Overall, I agree w/ breaking it out into it's own 0.x-versioned repo.

ryanschneider · 2019-07-08T21:29:14Z

+}
+
+// newNotification is a helper for creating encoded Request Messages without IDs.
+func newNotification(method string, params ...interface{}) (*Message, error) {


Why not add Notification as a public method and to the Requester interface? Or perhaps a new Notifier interface?

Mostly trying to reduce the public API surface. I imagine there are two use flows for this kind of library: Either you're using it at the highest level (Call(...)) and never have to think about the message types, or you're using it at the lowest level and passing around message instances through codecs.

These newNotification/newRequest helpers are kind-of in-between (really they're helpers for going from low-level to high-level). If you're constructing your own message types, you probably have enough custom stuff that you're not going to use this. If you're using the high-level interface, you're not going to use this either.

Also they're just the byproduct of an internal refactor, so it's not something that was exposed before.

ryanschneider · 2019-07-08T21:31:30Z

@@ -26,31 +52,121 @@ var _ Codec = &jsonCodec{}
 // IOCodec returns a Codec that wraps JSON encoding and decoding over IO.
 func IOCodec(rwc io.ReadWriteCloser) *jsonCodec {


Intentional discrepancy? Public function is IOCodec but it returns a jsonCodec?

All the *Codec helpers return a Codec interface implementation. In this case, it takes an IO (rwc) and returns a Codec (implemented by jsonCodec), so it's an IOCodec.

I don't return the Codec interface explicitly to comply with the "accept interfaces, return structs" mantra.

The fact that it's a jsonCodec is because it is json RPC after all. :)

I should probably:

Pull all these codecs into a codec sub-package, then could have jsonrpc2.codec.New(rwc)

Maybe make the jsonCodec struct a public JSONCodec so people can directly provide their own encoders/decoders.

I've been going back and forth on how I wanted the jsonCodec internals to be used, but it might be stable enough now.

ryanschneider · 2019-07-08T21:33:52Z

+	if len(batch) == 0 {
+		// FIXME: Should we be forgiving of this? Just return an empty message,
+		// or keep blocking until we get one?
+		return nil, errors.New("empty message batch")


According to the spec:

If the batch rpc call itself fails to be recognized as an valid JSON or as an Array with at least one value, the response from the Server MUST be a single Response object

https://www.jsonrpc.org/specification#batch

Also from the examples:

--> [] <-- {"jsonrpc": "2.0", "error": {"code": -32600, "message": "Invalid Request"}, "id": null}

Good catch on that in the specs, I missed it!

I'm preeeetty sure the outcome will be correct, because the error at the codec level will get converted into a JSONRPC error message at the transport level.

I should totally add a test for this.

Interestingly, it seems geth's jsonrpc implementation ignores empty batches.

In fact, I should add all the examples as tests.

You were right, needed to wire up the actual response error. (Edit: Got a few more changes coming up, hang tight)

ryanschneider · 2019-07-08T21:38:05Z

+		},
+		{
+			In: `
+				[


Is this actually desired behavior?

@ryanschneider How do you mean?

I'm probably missing something, but looks sensible to me. Note that the input is a JSON stream, so multiple messages are supported.

shazow · 2019-07-09T14:37:35Z

Leaning towards pulling this out into a standalone lib now, instead of even merging changes here.

shazow · 2019-07-09T14:37:54Z

Maybe take the opportunity to reduce the public API surface, too.

shazow · 2019-07-09T20:11:13Z

Going to move around where the batch errors are generated, and add a bunch more tests. Will ping again when it's ready for another look. :)

shazow and others added 9 commits July 8, 2019 13:35

Fix pool metadata bugs (#48)

47c6081

Fix pool metadata bugs

jsonrpc2: Add batch request/response support

65fe653

Needs more tests

jsonrpc2: More batch testing, switch jsonCodec to reuse the encoder/d…

173ff6e

…ecoder

jsonrpc2: Add TestReply, tweak message version

10007ac

jsonrpc2: Add Replier tests for batched replies

4422a3b

jsonrpc2: Wire up Server.Handler to use Reply for batch requests

14890aa

Mildly breaks the public API (probably unused)

jsonrpc2: Add TestRemoteBatch

797385e

jsonrpc2: Add Service.Notify

def76fe

jsonrpc2: Fix params encoding, add TestRemoteNotify and more

4d0695b

shazow force-pushed the jsonrpc2-batch branch from 2ae5c48 to 4d0695b Compare July 8, 2019 20:37

shazow commented Jul 8, 2019

View reviewed changes

shazow requested a review from ryanschneider July 8, 2019 20:48

ryanschneider approved these changes Jul 8, 2019

View reviewed changes

shazow added 2 commits July 9, 2019 14:55

jsonrpc2: Remove unused ErrorCode type

54b6b78

jsonrpc2: Write Invalid Request error response on empty batch

be3e614

shazow force-pushed the jsonrpc2-batch branch from bca7459 to be3e614 Compare July 9, 2019 18:59

		@@ -26,31 +52,121 @@ var _ Codec = &jsonCodec{}
		// IOCodec returns a Codec that wraps JSON encoding and decoding over IO.
		func IOCodec(rwc io.ReadWriteCloser) *jsonCodec {

Uh oh!

Conversation

shazow commented Jul 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanschneider left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shazow Jul 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shazow Jul 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shazow Jul 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shazow commented Jul 9, 2019

Uh oh!

shazow commented Jul 9, 2019

Uh oh!

shazow commented Jul 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

shazow commented Jul 5, 2019 •

edited

Loading

shazow Jul 9, 2019 •

edited

Loading

shazow Jul 9, 2019 •

edited

Loading

shazow Jul 9, 2019 •

edited

Loading