Skip to content

rpc/lib/client & server: try to conform to JSON-RPC 2.0 spec#4141

Merged
melekes merged 8 commits intomasterfrom
anton/3853-rpc-ids-2
Nov 15, 2019
Merged

rpc/lib/client & server: try to conform to JSON-RPC 2.0 spec#4141
melekes merged 8 commits intomasterfrom
anton/3853-rpc-ids-2

Conversation

@melekes
Copy link
Contributor

@melekes melekes commented Nov 14, 2019

Redo of #3858

https://www.jsonrpc.org/specification

What is done in this PR:

JSONRPCClient: validate that Response.ID matches Request.ID I wanted
to do the same for the WSClient, but since we're sending events as
responses, not notifications, checking IDs would require storing
them in memory indefinitely (and we won't be able to remove them
upon client unsubscribing because ID is different then).

Request.ID is now optional. Notification is a Request without an ID.
Previously "" or 0 were considered as notifications

Remove #event suffix from ID from an event response (partially fixes
#2949) ID must be either string, int or null AND must be equal to
request's ID. Now, because we've implemented events as responses, WS
clients are tripping when they see Response.ID("0#event") !=
Request.ID("0"). Implementing events as requests would require a lot
of time (~ 2 days to completely rewrite WS client and server)

generate unique ID for each request

switch to integer IDs instead of "json-client-XYZ"
id=0 method=/subscribe
id=0 result=...
id=1 method=/abci_query
id=1 result=...

send events (resulting from /subscribe) as requests+notifications (not
responses)

this will require a lot of work. probably not worth it

  • rpc: generate an unique ID for each request
    in conformance with JSON-RPC spec

  • WSClient: check for unsolicited responses

  • fix golangci warnings

  • save commit

  • fix errors

  • remove ID from responses from subscribe
    Fixes Responses to "subscribe" sent over WebSocket don't conform to RPC 2.0 specification #2949

  • clients are safe for concurrent access

  • tm-bench: switch to int ID

  • fixes after my own review

  • comment out sentIDs in WSClient
    see commit body for the reason

  • remove body.Close
    it will be closed automatically

  • stop ws connection outside of write/read routines
    also, use t.Rate in tm-bench indexer when calculating ID

  • fix gocritic issues

  • Referenced an issue explaining the need for the change
  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG_PENDING.md

https://www.jsonrpc.org/specification

What is done in this PR:

    JSONRPCClient: validate that Response.ID matches Request.ID I wanted
    to do the same for the WSClient, but since we're sending events as
    responses, not notifications, checking IDs would require storing
    them in memory indefinitely (and we won't be able to remove them
    upon client unsubscribing because ID is different then).

    Request.ID is now optional. Notification is a Request without an ID.
    Previously "" or 0 were considered as notifications

    Remove #event suffix from ID from an event response (partially fixes
    #2949) ID must be either string, int or null AND must be equal to
    request's ID. Now, because we've implemented events as responses, WS
    clients are tripping when they see Response.ID("0#event") !=
    Request.ID("0"). Implementing events as requests would require a lot
    of time (~ 2 days to completely rewrite WS client and server)

    generate unique ID for each request

    switch to integer IDs instead of "json-client-XYZ"
    id=0 method=/subscribe
    id=0 result=...
    id=1 method=/abci_query
    id=1 result=...

> send events (resulting from /subscribe) as requests+notifications (not
responses)

this will require a lot of work. probably not worth it

* rpc: generate an unique ID for each request
in conformance with JSON-RPC spec

* WSClient: check for unsolicited responses

* fix golangci warnings

* save commit

* fix errors

* remove ID from responses from subscribe
Fixes #2949

* clients are safe for concurrent access

* tm-bench: switch to int ID

* fixes after my own review

* comment out sentIDs in WSClient
see commit body for the reason

* remove body.Close
it will be closed automatically

* stop ws connection outside of write/read routines
also, use t.Rate in tm-bench indexer when calculating ID

fix gocritic issues
@melekes melekes force-pushed the anton/3853-rpc-ids-2 branch from dad7de0 to e6dcc81 Compare November 14, 2019 15:50
@melekes melekes added R:major PR contains breaking changes that have to wait till a major release is made to be merged T:breaking Type: Breaking Change labels Nov 14, 2019
@melekes melekes self-assigned this Nov 14, 2019
@codecov-io
Copy link

codecov-io commented Nov 14, 2019

Codecov Report

Merging #4141 into master will not change coverage.
The diff coverage is 37.5%.

@@           Coverage Diff           @@
##           master    #4141   +/-   ##
=======================================
  Coverage   66.68%   66.68%           
=======================================
  Files         247      247           
  Lines       21218    21221    +3     
=======================================
+ Hits        14149    14151    +2     
- Misses       6007     6013    +6     
+ Partials     1062     1057    -5
Impacted Files Coverage Δ
lite/proxy/wrapper.go 0% <0%> (ø) ⬆️
tools/tm-bench/transacter.go 8.49% <0%> (ø) ⬆️
rpc/core/events.go 0% <0%> (ø) ⬆️
rpc/client/httpclient.go 69.37% <100%> (-0.43%) ⬇️
p2p/pex/errors.go 12.5% <0%> (-12.5%) ⬇️
p2p/pex/pex_reactor.go 82.13% <0%> (-2.89%) ⬇️
blockchain/v0/reactor.go 77.83% <0%> (-0.95%) ⬇️
p2p/pex/addrbook.go 67.83% <0%> (-0.51%) ⬇️
consensus/state.go 77.53% <0%> (-0.22%) ⬇️
... and 7 more

Copy link
Contributor

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

after merge conflicts are resolved of course

@melekes melekes merged commit 44a3fbf into master Nov 15, 2019
@melekes melekes deleted the anton/3853-rpc-ids-2 branch November 15, 2019 10:16

// Read from the socket and subscribe to or unsubscribe from events
func (wsc *wsConnection) readRoutine() {
var request types.RPCRequest
Copy link

@NicolasMahe NicolasMahe Feb 14, 2020

Choose a reason for hiding this comment

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

a bug has been introduced by moving this variable outside of the for loop (because it is used in the defer function to display the request.ID in the error message).
When one ws client (one connection) wants to subscribe to multiple events (multiple call to Subscribe method), all ws messages contain the ID of the last subscriber.
previous place where this variable was declared: https://github.com/tendermint/tendermint/pull/4141/files#diff-692f910080346cdb0d29bfba93c21569L701

@antho1404 is working on a fix and should create a PR soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Thanks

Choose a reason for hiding this comment

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

PR in progress: #4406

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

R:major PR contains breaking changes that have to wait till a major release is made to be merged T:breaking Type: Breaking Change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Responses to "subscribe" sent over WebSocket don't conform to RPC 2.0 specification

5 participants