rpcserver: add batched request support (json 2.0) #841

dnldd · 2017-09-08T14:26:34Z

No description provided.

dnldd · 2017-09-10T18:29:15Z

@davecgh PR should be good now.

chappjc · 2017-09-13T00:46:48Z

Does this apply only to HTTP requests or also to websockets?

dnldd · 2017-09-13T00:50:21Z

@chappjc applies to websockets as well.

dajohi · 2017-09-13T16:55:10Z

rpcserver.go

-				jsonErr = rpcInvalidError("limited user not " +
-					"authorized for this method")
+		// determine if the request is single or batched
+		reqType := reflect.TypeOf(request)


instead of reflect, use switch/case:

switch request.(type) { case map[string]interface{}: ... }

dnldd · 2017-09-13T22:14:59Z

@dajohi updated the PR per your review.

dajohi · 2017-09-21T14:01:09Z

rpcserver.go

+		// determine if the request is single or batched
+		switch request.(type) {
+		case map[string]interface{}:
+			{


can you drop the scope and save on indentation?

sure thing, on it.

dnldd · 2017-09-21T14:22:38Z

@dajohi PR updated.

dajohi · 2017-09-22T13:05:50Z

rpcserver.go

+		switch request.(type) {
+		case map[string]interface{}:
+			request := request.(map[string]interface{})
+			req, err := dcrjson.NewRequest(request["id"].(interface{}), request["method"].(string), request["params"].([]interface{}))


please wrap at 80 columns

dajohi · 2017-09-22T13:06:00Z

rpcserver.go

+			batchedReply.WriteByte('[')
+			for idx, entry := range reqs {
+				entry := entry.(map[string]interface{})
+				req, err := dcrjson.NewRequest(entry["id"].(interface{}), entry["method"].(string), entry["params"].([]interface{}))


please wrap at 80 columns

dajohi · 2017-09-22T13:06:15Z

rpcwebsocket.go

+		switch request.(type) {
+		case map[string]interface{}:
+			data := request.(map[string]interface{})
+			req, err := dcrjson.NewRequest(data["id"].(interface{}), data["method"].(string), data["params"].([]interface{}))


please wrap at 80 columns

dajohi · 2017-09-22T13:06:42Z

rpcwebsocket.go

+			var replies [][]byte
+			for _, entry := range batch {
+				entry := entry.(map[string]interface{})
+				req, err := dcrjson.NewRequest(entry["id"].(interface{}), entry["method"].(string), entry["params"].([]interface{}))


please wrap at 80 columns

dnldd · 2017-09-22T21:40:40Z

@dajohi PR updated.

dajohi

Should be the last of my nitpicking

Looks good! 👍

dajohi · 2017-09-29T16:18:31Z

rpcserver.go

-				jsonErr = rpcInvalidError("limited user not " +
-					"authorized for this method")
+		// determine if the request is single or batched
+		switch request.(type) {


Could you add a default case that does something like

default: rpcslog.Debugf("unhandled request due to unknown type: %T", request)

dajohi · 2017-09-29T16:20:17Z

rpcwebsocket.go

-		if cmd.err != nil {
-			if !c.authenticated {
+		// Determine if the request is single request or a batched request
+		switch request.(type) {


Could you add a default case that does something like

default: rpcslog.Debugf("unhandled request due to unknown type: %T", request)

sure, on it.

dnldd · 2017-09-29T23:20:00Z

@dajohi PR updated.

davecgh

Unfortunately, this can't go in as is. There are a lot of issues such as failing to check error returns, the ability to crash it due to naked type casts, inefficiencies due to improper use of interfaces instead of unmarshalling into the appropriate dcrjson.Result type, and not following the spec for malformed requests.

davecgh · 2017-10-11T00:25:50Z

rpcwebsocket.go

-// QueueNotification which implements a queue via notificationQueueHandler to
-// ensure sending notifications from other subsystems can't block.  Ultimately,
-// all messages are sent via the outHandler.
+// wsClient provides an abstraction for handling a websocket client.


This comment is not wrapped at 80 cols properly.

It should be:

// wsClient provides an abstraction for handling a websocket client. The // overall data flow is split into 3 main goroutines. A websocket manager is // used to allow things such as broadcasting requested notifications to all // connected websocket clients. Inbound messages are read via the inHandler // goroutine and generally dispatched to their own handler. There are two // outbound message types - one for responding to client requests and another // for async notifications. Responses to client requests use SendMessage which // employs a buffered channel thereby limiting the number of outstanding // requests that can be made. Notifications are sent via QueueNotification // which implements a queue via notificationQueueHandler to ensure sending // notifications from other subsystems can't block. Ultimately, all messages // are sent via the outHandler.

Good editors are capable of doing this wrapping for you if they're configured properly.

will address the issues pointed out, thanks.

davecgh · 2017-10-15T21:06:15Z

rpcserver.go

-	var result interface{}
-	var request dcrjson.Request
+	var msg []byte
+	var request interface{}


To clarify a bit on what I mean about interfaces, notice the code here is doing the unmarshal into an interface. Instead, it would be more efficient and safer to look at the byte stream in order to determine whether it should unmarshal into a Request or a []Request in order to be properly typed.

For example:

// At package scope... var batchedRequestPrefix = []byte("[{") ... if bytes.HasPrefix(body, batchedRequestPrefix) { /* attempt unmarshal to []Request */ } else { /* attempt unmarshal to Request */ }

EDIT: For a bit more of an update on this, upon looking at the specification again, I see that each individual request within the batch requires an individual response unless the entire JSON fails to parse. That means the initial unmarshal will indeed need to go into a []interface, however, every element within that array should be unmarshalled into a Request instead of manually trying to do naked type casts.

Here are the three relevant examples from the spec:

rpc call Batch, invalid JSON:

--> [ {"jsonrpc": "2.0", "method": "sum", "params": [1,2,4], "id": "1"}, {"jsonrpc": "2.0", "method" ] <-- {"jsonrpc": "2.0", "error": {"code": -32700, "message": "Parse error"}, "id": null}

rpc call with invalid Batch, but valid JSON:

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

rpc call with mixed valid and invalid requests, but all valid JSON:

--> [ {"jsonrpc": "2.0", "method": "sum", "params": [1,2,4], "id": "1"}, {"jsonrpc": "2.0", "method": "notify_hello", "params": [7]}, {"jsonrpc": "2.0", "method": "subtract", "params": [42,23], "id": "2"}, {"foo": "boo"}, {"jsonrpc": "2.0", "method": "foo.get", "params": {"name": "myself"}, "id": "5"}, {"jsonrpc": "2.0", "method": "get_data", "id": "9"} ] <-- [ {"jsonrpc": "2.0", "result": 7, "id": "1"}, {"jsonrpc": "2.0", "result": 19, "id": "2"}, {"jsonrpc": "2.0", "error": {"code": -32600, "message": "Invalid Request"}, "id": null}, {"jsonrpc": "2.0", "error": {"code": -32601, "message": "Method not found"}, "id": "5"}, {"jsonrpc": "2.0", "result": ["hello", 5], "id": "9"} ]

davecgh · 2017-10-15T21:12:24Z

rpcserver.go

+			batchedReply.WriteByte('[')
+			for idx, entry := range reqs {
+				entry := entry.(map[string]interface{})
+				req, err := dcrjson.NewRequest(entry["id"].(interface{}),


This is an example of what I meant by naked type casts that can crash.

Notice what would happen if you fed this code requests like:

[{"jsonrpc":1.0", "bogus":"foo"}] or [{"jsonrpc":1.0", "id":"1", params:"foo"}]

dnldd · 2017-10-19T23:36:55Z

@davecgh I've updated the http post implementation, the websocket impl. will be up in a couple of days. Would like a review for the http post impl. in the mean time when you're free, thanks.

dnldd · 2017-10-21T03:42:18Z

updated, should be good now.

issues addressed

davecgh

This is producing some incorrect results in some negative paths. See the following code:

package main

import (
	"bytes"
	"crypto/tls"
	"crypto/x509"
	"fmt"
	"io/ioutil"
	"net/http"
	"path/filepath"

	"github.com/decred/dcrd/dcrutil"
)

func main() {
	const (
		url     = "https://127.0.0.1:19109"
		rpcUser = "yourrpcuser"
		rpcPass = "yourrpcpass"
	)

	// populate request set
	reqs := []string{
		`{}`,
		`[]`,
		`[1]`,
		`[1,2,3]`,
		`{"foo": "boo"}`,                              // should be an invalid request
		`{"jsonrpc": "1.0", "foo": "boo", "id": "1"}`,
		`{"jsonrpc": "1.0", "method": "getblockcount", "params": [], "id": "1"}`,
		`{"jsonrpc": "1.0", "method": "getblockcount", "params": "a", "id": "1"}`, // should be invalid since params is neither an array nor a json object.
		`[
			{"jsonrpc": "2.0", "method": "getblockcount", "params": [], "id": "1"},
			{"jsonrpc": "2.0", "method": "decodescript", "params": ["ac"]},
			{"jsonrpc": "2.0", "method": "getbestblockhash", "params": [], "id": "2"},
			{"foo": "boo"},
			{"jsonrpc": "2.0", "method": "getblockcount", "id": "9"} 
		]`, // should produce invalid request for the `{"foo": "boo"}`.
	}

	// Connect to local dcrd RPC server using websockets.
	dcrdHomeDir := dcrutil.AppDataDir("dcrd", false)
	certs, err := ioutil.ReadFile(filepath.Join(dcrdHomeDir, "rpc.cert"))
	if err != nil {
		fmt.Println(err)
		return
	}

	pool := x509.NewCertPool()
	pool.AppendCertsFromPEM(certs)
	client := http.Client{
		Transport: &http.Transport{
			TLSClientConfig: &tls.Config{
				RootCAs: pool,
			},
		},
	}

	for _, jsonReq := range reqs {
		bodyReader := bytes.NewReader([]byte(jsonReq))
		httpReq, err := http.NewRequest("POST", url, bodyReader)
		if err != nil {
			fmt.Println(err)
			return
		}
		httpReq.Close = true
		httpReq.Header.Set("Content-Type", "application/json")
		httpReq.SetBasicAuth(rpcUser, rpcPass)
		resp, err := client.Do(httpReq)
		if err != nil {
			fmt.Println("request:", jsonReq, "response:", err)
			return
		}

		respBytes, err := ioutil.ReadAll(resp.Body)
		resp.Body.Close()
		if err != nil {
			fmt.Println(err)
			return
		}

		fmt.Println("request:", jsonReq, "response:", string(respBytes))
	}
}

Output:

request: {} response: 
request: [] response: {"jsonrpc":"1.0","result":null,"error":{"code":-32600,"message":"Invalid request: empty batch"},"id":null}
request: [1] response: [{"jsonrpc":"1.0","result":null,"error":{"code":-32600,"message":"Invalid request: json: cannot unmarshal number into Go value of type dcrjson.Request"},"id":null}]
request: [1,2,3] response: [{"jsonrpc":"1.0","result":null,"error":{"code":-32600,"message":"Invalid request: json: cannot unmarshal number into Go value of type dcrjson.Request"},"id":null},{"jsonrpc":"1.0","result":null,"error":{"code":-32600,"message":"Invalid request: json: cannot unmarshal number into Go value of type dcrjson.Request"},"id":null},{"jsonrpc":"1.0","result":null,"error":{"code":-32600,"message":"Invalid request: json: cannot unmarshal number into Go value of type dcrjson.Request"},"id":null}]
request: {"foo": "boo"} response: 
request: {"jsonrpc": "1.0", "foo": "boo", "id": "1"} response: {"jsonrpc":"1.0","result":null,"error":{"code":-32601,"message":"Method not found"},"id":"1"}
request: {"jsonrpc": "1.0", "method": "getblockcount", "params": [], "id": "1"} response: {"jsonrpc":"1.0","result":227411,"error":null,"id":"1"}
request: {"jsonrpc": "1.0", "method": "getblockcount", "params": "a", "id": "1"} response: {"jsonrpc":"1.0","result":227411,"error":null,"id":"1"}
request: [
	{"jsonrpc": "2.0", "method": "getblockcount", "params": [], "id": "1"},
	{"jsonrpc": "2.0", "method": "decodescript", "params": ["ac"]},
	{"jsonrpc": "2.0", "method": "getbestblockhash", "params": [], "id": "2"},
	{"foo": "boo"},
	{"jsonrpc": "2.0", "method": "getblockcount", "id": "9"} 
] response: [{"jsonrpc":"2.0","result":227411,"error":null,"id":"1"},{"jsonrpc":"2.0","result":"0000000003563140b1bd52599ac1abca98900bce048e9c3ba9ca72eba948f7d6","error":null,"id":"2"},{"jsonrpc":"2.0","result":227411,"error":null,"id":"9"}]

davecgh · 2018-01-28T21:55:31Z

dcrjson/jsonrpc.go

-// Response is the general form of a JSON-RPC response.  The type of the Result
-// field varies from one command to the next, so it is implemented as an
-// interface.  The ID field has to be a pointer for Go to put a null in it when
+// Response is the general form of a JSON-RPC 1.0 response.  The type of the


Why the addition of 1.0? This applies to both versions.

davecgh · 2018-01-29T19:21:20Z

The final {"jsonrpc": "2.0", "method": "getblockcount", "id": "9"} in the batch is incorrectly failing. That should be valid because it doesn't specify params at all which is acceptable.

davecgh · 2018-01-29T23:51:14Z

dcrjson/jsonrpc.go

-// type with the NewCmd or New<Foo>Cmd functions and call the MarshalCmd
-// function with that command to generate the marshalled JSON-RPC request.
-func NewRequest(id interface{}, method string, params []interface{}) (*Request, error) {
+func (request *Request) UnmarshalJSON(b []byte) error {


Missing function comment.

davecgh · 2018-01-30T00:10:11Z

rpcserver.go

+}
+
+// processRequest determines incoming request type (single or batched),
+// parses it and returns a response.


s/response/marshalled response/

davecgh · 2018-01-30T00:41:47Z

rpcwebsocket.go

-			if err != nil {
-				rpcsLog.Errorf("Failed to marshal parse failure "+
-					"reply: %v", err)
+			cmd := parseCmd(&req)


Seems like this should retain the previous functionality of checking for req.ID == nil first to avoid the extra overhead of parsing notifications. I agree that this is correct behavior due to the addition of the check for nil in the parseCmd function, but there is non reason to even create the struct and churn GC for notifications.

davecgh · 2018-01-30T00:43:13Z

rpcwebsocket.go

-					Code:    dcrjson.ErrRPCInvalidParams.Code,
-					Message: "limited user not authorized for this method",
+			rpcsLog.Debugf("Received command <%s> from %s", cmd.method, c.addr)
+			// Check auth.  The client is immediately disconnected if the


Needs a blank line before this to separate the logical blocks.

jakesylvestre · 2019-12-04T06:31:01Z

Is there an example of using this on an http post?

davecgh · 2019-12-04T07:34:20Z

It's the same as a regular request except you create an array of of them:

e.g.

$ curl --silent --cacert "~/.dcrd/rpc.cert" --user "USER:PASS" --data-binary '[{"jsonrpc": "1.0", "id":1, "method": "getinfo", "params": []},{"jsonrpc": "1.0", "id":2, "method": "getblockcount", "params": []}]' https://127.0.0.1:9109
[{"jsonrpc":"1.0","result":{"version":1060000,"protocolversion":7,"blocks":402789,"timeoffset":38,"connections":8,"proxy":"","difficulty":27376557113.1791,"testnet":false,"relayfee":0.0001,"errors":""},"error":null,"id":1},{"jsonrpc":"1.0","result":402789,"error":null,"id":2}]

jakesylvestre · 2019-12-05T07:40:43Z

Got it. @davecgh, this was tremendously helpful in putting together the PR mentioned above. By the way, you might find this bulk query builder helpful for this project.

dnldd force-pushed the json2 branch from 13cae2b to fd73aa6 Compare September 10, 2017 18:17

dajohi requested changes Sep 13, 2017

View reviewed changes

dnldd force-pushed the json2 branch from fd73aa6 to 8a73b55 Compare September 13, 2017 22:04

dajohi reviewed Sep 21, 2017

View reviewed changes

dnldd force-pushed the json2 branch from 8a73b55 to 0968ec7 Compare September 21, 2017 14:12

dajohi reviewed Sep 22, 2017

View reviewed changes

dnldd force-pushed the json2 branch from 0968ec7 to 0ef88dd Compare September 22, 2017 21:40

dajohi previously requested changes Sep 29, 2017

View reviewed changes

dnldd force-pushed the json2 branch from 0ef88dd to c2a690d Compare September 29, 2017 23:07

davecgh requested changes Oct 11, 2017

View reviewed changes

davecgh requested changes Oct 15, 2017

View reviewed changes

dnldd force-pushed the json2 branch 5 times, most recently from ba4f6ae to 79ec1b9 Compare October 19, 2017 23:22

dnldd force-pushed the json2 branch from afb94e0 to 6fa5ce3 Compare October 21, 2017 03:30

davecgh mentioned this pull request Nov 9, 2017

[rpc] Add support for JSON-RPC 2.0 batch requests btcsuite/btcd#329

Closed

davecgh requested changes Jan 28, 2018

View reviewed changes

dnldd force-pushed the json2 branch from 98ed2b9 to c3d5a40 Compare January 29, 2018 00:33

dnldd force-pushed the json2 branch 2 times, most recently from 3a2040f to 5b4cafd Compare January 30, 2018 00:08

davecgh requested changes Jan 30, 2018

View reviewed changes

dnldd force-pushed the json2 branch 2 times, most recently from a819423 to 949b22b Compare February 1, 2018 11:46

rpcserver: add batched request support (json 2.0)

406d873

dnldd force-pushed the json2 branch from 949b22b to 406d873 Compare February 1, 2018 11:47

davecgh approved these changes Feb 13, 2018

View reviewed changes

davecgh merged commit a0438c8 into decred:master Feb 13, 2018

TrantorRWS mentioned this pull request Feb 14, 2018

batch request issues (json 2.0) #1029

Closed

davecgh mentioned this pull request Mar 6, 2018

[RPC] Add support for JSON-RPC 2.0 batch requests #40

Closed

dnldd deleted the json2 branch June 4, 2018 23:26

erasmospunk mentioned this pull request Aug 5, 2018

rpcserver: add batched request support HcashOrg/hcd#4

Closed

jakesylvestre mentioned this pull request Dec 4, 2019

Add Batch JSON-RPC support (rpc client & server) btcsuite/btcd#1503

Merged

This was referenced May 22, 2020

Add Batch JSON-RPC support (rpc client & server) btcsuite/btcd#1582

Closed

Add Batch JSON-RPC support (rpc client & server) btcsuite/btcd#1583

Merged

rpcserver: add batched request support (json 2.0) #841

rpcserver: add batched request support (json 2.0) #841

Conversation

dnldd commented Sep 8, 2017

dnldd commented Sep 10, 2017

chappjc commented Sep 13, 2017

dnldd commented Sep 13, 2017

Choose a reason for hiding this comment

dnldd commented Sep 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnldd commented Sep 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnldd commented Sep 22, 2017

dajohi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnldd commented Sep 29, 2017

davecgh left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davecgh Oct 15, 2017 • edited Loading

Choose a reason for hiding this comment

davecgh Oct 15, 2017 • edited Loading

Choose a reason for hiding this comment

dnldd commented Oct 19, 2017 • edited Loading

dnldd commented Oct 21, 2017

davecgh left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davecgh commented Jan 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakesylvestre commented Dec 4, 2019

davecgh commented Dec 4, 2019

jakesylvestre commented Dec 5, 2019

davecgh left a comment •

edited

Loading

davecgh Oct 15, 2017 •

edited

Loading

davecgh Oct 15, 2017 •

edited

Loading

dnldd commented Oct 19, 2017 •

edited

Loading

davecgh left a comment •

edited

Loading