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

rpc: Add support for batched requests/responses #3534

Merged
merged 40 commits into from
Apr 17, 2019
Merged

rpc: Add support for batched requests/responses #3534

merged 40 commits into from
Apr 17, 2019

Conversation

thanethomson
Copy link
Contributor

@thanethomson thanethomson commented Apr 8, 2019

Continues from #3280 in building support for batched requests/responses in the JSON RPC (as per issue #3213).

  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG_PENDING.md

@thanethomson thanethomson changed the title [WIP] Add support for batched requests/responses in JSON RPC Add support for batched requests/responses in JSON RPC Apr 8, 2019
@codecov-io
Copy link

codecov-io commented Apr 8, 2019

Codecov Report

Merging #3534 into develop will decrease coverage by 0.14%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #3534      +/-   ##
===========================================
- Coverage    64.17%   64.02%   -0.15%     
===========================================
  Files          213      213              
  Lines        17373    17388      +15     
===========================================
- Hits         11149    11133      -16     
- Misses        5301     5323      +22     
- Partials       923      932       +9
Impacted Files Coverage Δ
rpc/client/helpers.go 87.5% <100%> (ø) ⬆️
rpc/client/httpclient.go 68.48% <100%> (+2.86%) ⬆️
privval/signer_service_endpoint.go 85.45% <0%> (-3.64%) ⬇️
consensus/reactor.go 70.36% <0%> (-2.6%) ⬇️
privval/signer_remote.go 80% <0%> (-2%) ⬇️
libs/clist/clist.go 66.66% <0%> (-1.52%) ⬇️
consensus/state.go 78.82% <0%> (-0.59%) ⬇️
libs/db/remotedb/grpcdb/server.go 0% <0%> (ø) ⬆️
p2p/pex/pex_reactor.go 79.44% <0%> (+0.61%) ⬆️

rpc/client/httpclient.go Outdated Show resolved Hide resolved
rpc/client/httpclient.go Outdated Show resolved Hide resolved
rpc/client/httpclient.go Outdated Show resolved Hide resolved
rpc/client/rpc_test.go Show resolved Hide resolved
rpc/lib/server/http_server.go Show resolved Hide resolved
rpc/lib/client/http_client.go Outdated Show resolved Hide resolved
rpc/client/httpclient.go Outdated Show resolved Hide resolved
@xla xla changed the title Add support for batched requests/responses in JSON RPC rpc: Add support for batched requests/responses Apr 9, 2019
@thanethomson thanethomson added the C:rpc Component: JSON RPC, gRPC label Apr 9, 2019
This refactors the batching functionality to rather act in a standalone
way. In light of supporting concurrent goroutines making use of the same
client, it would make sense to have batching functionality where one
could create a batch of requests per goroutine and send that batch
without interfering with a batch from another goroutine.
rpc/client/examples_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@liamsi liamsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks great 👍

rpc/client/examples_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@liamsi liamsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all changes since the last review. Still LGTM and it seems all comments were addressed 👍 I would wait for @melekes approval or further comments here before merging.

CHANGELOG_PENDING.md Outdated Show resolved Hide resolved
rpc/test/helpers.go Show resolved Hide resolved
This adds code to automatically generate a randomized client ID for the
JSONRPCClient, and adds a check of the IDs in the responses (if one was
set in the requests).
rpc/client/httpclient.go Outdated Show resolved Hide resolved
rpc/client/httpclient.go Outdated Show resolved Hide resolved
rpc/client/httpclient.go Outdated Show resolved Hide resolved
rpc/client/httpclient.go Outdated Show resolved Hide resolved
rpc/lib/client/http_client.go Outdated Show resolved Hide resolved
rpc/lib/client/http_client.go Outdated Show resolved Hide resolved
rpc/lib/client/http_client.go Outdated Show resolved Hide resolved
rpc/lib/client/http_client.go Outdated Show resolved Hide resolved
rpc/lib/server/handlers.go Outdated Show resolved Hide resolved
rpc/lib/server/handlers.go Outdated Show resolved Hide resolved
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧀 🍷

@melekes melekes merged commit 90465f7 into develop Apr 17, 2019
@melekes melekes deleted the issues/3213 branch April 17, 2019 15:10
@melekes melekes mentioned this pull request May 7, 2019
36 tasks
brapse pushed a commit to brapse/tendermint that referenced this pull request Jun 5, 2019
Continues from tendermint#3280 in building support for batched requests/responses in the JSON RPC (as per issue tendermint#3213).

* Add JSON RPC batching for client and server

As per tendermint#3213, this adds support for [JSON RPC batch requests and
responses](https://www.jsonrpc.org/specification#batch).

* Add additional checks to ensure client responses are the same as results

* Fix case where a notification is sent and no response is expected

* Add test to check that JSON RPC notifications in a batch are left out in responses

* Update CHANGELOG_PENDING.md

* Update PR number now that PR has been created

* Make errors start with lowercase letter

* Refactor batch functionality to be standalone

This refactors the batching functionality to rather act in a standalone
way. In light of supporting concurrent goroutines making use of the same
client, it would make sense to have batching functionality where one
could create a batch of requests per goroutine and send that batch
without interfering with a batch from another goroutine.

* Add examples for simple and batch HTTP client usage

* Check errors from writer and remove nolinter directives

* Make error strings start with lowercase letter

* Refactor examples to make them testable

* Use safer deferred shutdown for example Tendermint test node

* Recompose rpcClient interface from pre-existing interface components

* Rename WaitGroup for brevity

* Replace empty ID string with request ID

* Remove extraneous test case

* Convert first letter of errors.Wrap() messages to lowercase

* Remove extraneous function parameter

* Make variable declaration terse

* Reorder WaitGroup.Done call to help prevent race conditions in the face of failure

* Swap mutex to value representation and remove initialization

* Restore empty JSONRPC string ID in response to prevent nil

* Make JSONRPCBufferedRequest private

* Revert PR hard link in CHANGELOG_PENDING

* Add client ID for JSONRPCClient

This adds code to automatically generate a randomized client ID for the
JSONRPCClient, and adds a check of the IDs in the responses (if one was
set in the requests).

* Extract response ID validation into separate function

* Remove extraneous comments

* Reorder fields to indicate clearly which are protected by the mutex

* Refactor for loop to remove indexing

* Restructure and combine loop

* Flatten conditional block for better readability

* Make multi-variable declaration slightly more readable

* Change for loop style

* Compress error check statements

* Make function description more generic to show that we support different protocols

* Preallocate memory for request and result objects
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:rpc Component: JSON RPC, gRPC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants