rpc/lib/client & server: try to conform to JSON-RPC 2.0 spec by melekes · Pull Request #3858 · tendermint/tendermint

melekes · 2019-07-31T20:43:50Z

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 Responses to "subscribe" sent over WebSocket don't conform to RPC 2.0 specification #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=...

TODO:

[ ] send events (resulting from /subscribe) as notifications (not responses) https://github.com/ethereum/go-ethereum/wiki/RPC-PUB-SUB

tendermint/rpc/core/events.go

Lines 180 to 185 in 2f32b9f

ctx.WSConn.TryWriteRPCResponse(

rpctypes.NewRPCSuccessResponse(

ctx.WSConn.Codec(),

ctx.JSONReq.ID,

resultEvent,

))

From spec: "A Notification is a Request object"

check tm-bench works

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

in conformance with JSON-RPC spec

rpc/lib/types/types.go

rpc/lib/client/ws_client.go

rpc/lib/server/ws_handler_test.go

Fixes #2949

codecov-io · 2019-08-01T12:28:08Z

Codecov Report

❗ No coverage uploaded for pull request base (master@f9cce28). Click here to learn what that means.
The diff coverage is 41.62%.

@@            Coverage Diff            @@
##             master    #3858   +/-   ##
=========================================
  Coverage          ?   65.92%           
=========================================
  Files             ?      240           
  Lines             ?    20297           
  Branches          ?        0           
=========================================
  Hits              ?    13381           
  Misses            ?     5861           
  Partials          ?     1055

Impacted Files	Coverage Δ
rpc/core/events.go	`0% <0%> (ø)`
tools/tm-bench/transacter.go	`8.49% <0%> (ø)`
rpc/lib/server/http_server.go	`63.36% <0%> (ø)`
rpc/lib/client/http_uri_client.go	`0% <0%> (ø)`
rpc/lib/client/http_json_client.go	`9.64% <21.56%> (ø)`
rpc/lib/server/ws_handler.go	`35.32% <35.32%> (ø)`
rpc/lib/server/http_uri_handler.go	`42.42% <42.42%> (ø)`
rpc/lib/client/encode.go	`47.82% <47.82%> (ø)`
rpc/lib/client/decode.go	`5.08% <5.08%> (ø)`
rpc/lib/client/ws_client.go	`58.77% <60%> (ø)`
... and 2 more

melekes · 2019-08-01T13:15:32Z

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

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

rpc/lib/client/decode.go

rpc/lib/client/ws_client.go

rpc/lib/server/http_json_handler.go

melekes · 2019-08-01T13:46:41Z

rpc/lib/server/ws_handler.go

+		cdc:     cdc,
+		Upgrader: websocket.Upgrader{
+			CheckOrigin: func(r *http.Request) bool {
+				// TODO ???


The default behaviour would be relevant to browser-based clients, afaik. I suppose having a pass-through is a workaround for allowing for more complex security schemes, shifting the burden of AuthN/AuthZ outside the Tendermint RPC.

I can't think of other uses right now that would warrant a TODO though. The real backstory of this TODO shall remain shrouded in mystery 👻

Gold! Saved this comment for future generations 😄

see commit body for the reason

it will be closed automatically

tools/tm-bench/transacter.go

rpc/lib/server/ws_handler.go

also, use t.Rate in tm-bench indexer when calculating ID

tac0turtle · 2019-08-07T10:09:00Z

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

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

I do believe this is worth it, to bring us within the spec and what the wider community is using and used to. Do you think we could have it for 0.33? we can branch off from this branch then make the PR to be merged into this PR as it has already been reviewed?

melekes · 2019-11-14T13:24:13Z

Impossible to merge master (too many conflicts). Will have to redo every step.

melekes added 2 commits August 1, 2019 00:43

rpc: generate an unique ID for each request

d118c48

in conformance with JSON-RPC spec

WSClient: check for unsolicited responses

b02a340

golangcibot reviewed Aug 1, 2019

View reviewed changes

rpc/lib/types/types.go Outdated Show resolved Hide resolved

rpc/lib/client/ws_client.go Outdated Show resolved Hide resolved

melekes added 3 commits August 1, 2019 12:44

fix golangci warnings

019ebbd

save commit

0a0c39c

fix errors

9c79fad

golangcibot reviewed Aug 1, 2019

View reviewed changes

rpc/lib/server/ws_handler_test.go Show resolved Hide resolved

remove ID from responses from subscribe

34fd200

Fixes #2949

melekes changed the title ~~rpc: generate an unique ID for each request~~ rpc/lib/client & server: conform to JSON-RPC 2.0 spec Aug 1, 2019

melekes self-assigned this Aug 1, 2019

melekes added 2 commits August 1, 2019 16:18

clients are safe for concurrent access

2f32b9f

tm-bench: switch to int ID

fc4a98e

melekes mentioned this pull request Aug 1, 2019

Issues after upgrading from 0.31.5 -> 0.31.8 #3853

Closed

melekes commented Aug 1, 2019

View reviewed changes

melekes added 2 commits August 1, 2019 17:58

fixes after my own review

c2e1970

comment out sentIDs in WSClient

ec452e0

see commit body for the reason

melekes added the T:breaking Type: Breaking Change label Aug 1, 2019

melekes marked this pull request as ready for review August 1, 2019 14:09

melekes requested review from ebuchman and xla as code owners August 1, 2019 14:09

melekes changed the title ~~rpc/lib/client & server: conform to JSON-RPC 2.0 spec~~ rpc/lib/client & server: try to conform to JSON-RPC 2.0 spec Aug 1, 2019

remove body.Close

23287bd

it will be closed automatically

thanethomson approved these changes Aug 1, 2019

View reviewed changes

tools/tm-bench/transacter.go Outdated Show resolved Hide resolved

rpc/lib/server/ws_handler.go Show resolved Hide resolved

rpc/lib/server/ws_handler.go Outdated Show resolved Hide resolved

rpc/lib/server/ws_handler.go Outdated Show resolved Hide resolved

melekes added 3 commits August 2, 2019 10:35

stop ws connection outside of write/read routines

ad15ca5

also, use t.Rate in tm-bench indexer when calculating ID

Merge branch 'master' into anton/3853-rpc-ids

1904e77

fix gocritic issues

a11fd8c

melekes mentioned this pull request Aug 2, 2019

style: consider using file structure instead of ASCI lines #3619

Closed

melekes added the R:major PR contains breaking changes that have to wait till a major release is made to be merged label Aug 5, 2019

This was referenced Aug 14, 2019

Close socket on panic() iov-one/weave#851

Closed

Upgrade to future tendermint 0.33 and test that it does not break the rpc client anymore iov-one/weave#976

Closed

melekes closed this Nov 14, 2019

melekes mentioned this pull request Nov 14, 2019

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

Merged

5 tasks

melekes deleted the anton/3853-rpc-ids branch November 15, 2019 14:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

rpc/lib/client & server: try to conform to JSON-RPC 2.0 spec#3858
melekes wants to merge 14 commits intomasterfrom
anton/3853-rpc-ids

melekes commented Jul 31, 2019 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-io commented Aug 1, 2019 •

edited

Loading

Uh oh!

melekes commented Aug 1, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

melekes Aug 1, 2019

Uh oh!

thanethomson Aug 1, 2019

Uh oh!

melekes Aug 2, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tac0turtle commented Aug 7, 2019

Uh oh!

melekes commented Nov 14, 2019 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	ctx.WSConn.TryWriteRPCResponse(
	rpctypes.NewRPCSuccessResponse(
	ctx.WSConn.Codec(),
	ctx.JSONReq.ID,
	resultEvent,
	))

Conversation

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-io commented Aug 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

melekes commented Aug 1, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

melekes Aug 1, 2019

Choose a reason for hiding this comment

Uh oh!

thanethomson Aug 1, 2019

Choose a reason for hiding this comment

Uh oh!

melekes Aug 2, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tac0turtle commented Aug 7, 2019

Uh oh!

melekes commented Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

melekes commented Jul 31, 2019 •

edited

Loading

codecov-io commented Aug 1, 2019 •

edited

Loading

melekes commented Nov 14, 2019 •

edited

Loading