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.

Sign up for GitHub

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

Jump to bottom

Add IPC transport for JSON-RPC APIs #3695

Merged
merged 8 commits into from
Apr 27, 2022
Merged

Add IPC transport for JSON-RPC APIs #3695

merged 8 commits into from
Apr 27, 2022

Conversation

diega
Copy link
Contributor

@diega diega commented Apr 5, 2022 •
edited
Loading

PR description

Adds socket interface for JSON-RPC, only supported on BSD (OSX) and Linux. This feature has been developed with the intention to reuse and unify, as much as possible, the behaviour among the other transport layers (i.e. HTTP and WS)

Given this is a big PR to review, I have split it in the following commits (I recommend checking them individually):

  • HEAD~7: Fixes two tests for the HTTP implementation (more info in the commit message)
  • HEAD~6: Extracts HTTP authentication to its own handler
  • HEAD~5: Moves authentication handler first in the pipeline, to fail fast (*)
  • HEAD~4: Replaces timeout handler in GraphQL service to use the one provided by Vert.x (*)
  • HEAD~3: Extracts HTTP json parse to its own handler
  • HEAD~2: Extracts json-rpc execution logic outside of the HTTP handler
  • HEAD~1: Refactor WS service to use the same execution logic as the HTTP service (*)
  • HEAD: Implements IPC transport layer

(*): these commits can be omitted without affecting the implementation of the IPC transport

Note

This implementation has been tested against geth attach and it works fine.

Fixed Issue(s)

fixes #535

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if
    updates are required.

Changelog

@diega diega added the doc-change-required Indicates an issue or PR that requires doc to be updated label Apr 5, 2022
@diega diega force-pushed the jsonrpc_ipc branch 10 times, most recently from a2b6e2b to 229ba99 Compare April 8, 2022 13:26
@hyperledger hyperledger deleted a comment from lgtm-com bot Apr 8, 2022
@diega diega marked this pull request as ready for review April 8, 2022 13:56
@hyperledger hyperledger deleted a comment from lgtm-com bot Apr 9, 2022
atoulme
atoulme approved these changes Apr 12, 2022
View reviewed changes
Copy link
Contributor

@atoulme atoulme left a comment

Choose a reason for hiding this comment

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

LGTM

@diega diega force-pushed the jsonrpc_ipc branch 3 times, most recently from fb4927b to e042cc9 Compare April 15, 2022 00:06
@hyperledger hyperledger deleted a comment from lgtm-com bot Apr 15, 2022
@hyperledger hyperledger deleted a comment from lgtm-com bot Apr 15, 2022
shemnon
shemnon approved these changes Apr 15, 2022
View reviewed changes
Copy link
Contributor

@shemnon shemnon left a comment

Choose a reason for hiding this comment

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

I like the idea of it, but it's been so long since I've been in that code I'm not sure what is an improvement and what isn't. The acceptance tests still pass.

A set of acceptance tests that use IPC to do some testing wold be nice, but doesn't have to be in this PR and can be in a follow on. It's also tricky because it may have OS specific setup.

Generally speaking a quarterly release is the right time to put these changes in, as we've signaled that's when breaking features come in. While re-writing core parts of JSON aren't breaking, they are higher risk.

Unless @macfarla, @usmansaleem , @mark-terry, or someone else whose had their hands deep in the RPC code chime in I'm cool with @diega pressing the merge button before RC2 gets spun out.

@hyperledger hyperledger deleted a comment from lgtm-com bot Apr 15, 2022
@macfarla
Copy link
Contributor

@mark-terry can you review?

@alexandratran alexandratran mentioned this pull request Apr 20, 2022
Closed
@alexandratran alexandratran removed the doc-change-required Indicates an issue or PR that requires doc to be updated label Apr 20, 2022
antonydenyer
antonydenyer requested changes Apr 22, 2022
View reviewed changes
Copy link
Member

@antonydenyer antonydenyer left a comment

Choose a reason for hiding this comment

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

Really excited to see this implemented. Great stuff.

...m/api/src/test/java/org/hyperledger/besu/ethereum/api/jsonrpc/ipc/JsonRpcIpcServiceTest.java Outdated Show resolved Hide resolved
@hyperledger hyperledger deleted a comment from lgtm-com bot Apr 25, 2022
@hyperledger hyperledger deleted a comment from lgtm-com bot Apr 25, 2022
@hyperledger hyperledger deleted a comment from lgtm-com bot Apr 25, 2022
@macfarla
Copy link
Contributor

@diega this needs a changelog entry :)

@diega
Fix json-rpc HTTP tests [hyperledger#535]
8fc185e 
The `o.h.b.e.a.j.JsonRpcHttpServiceTest#exceptionallyHandleJsonSingleRequest` and `o.h.b.e.a.j.JsonRpcHttpServiceTest#exceptionallyHandleJsonBatchRequest` tests were throwing ClassCastException in `o.h.b.e.a.j.JsonRpcHttpService#validateMethodAvailability` which wasn't ever catched, returning status 500 by default, but that wasn't the use case aimed to test. Another test running an exceptional method is `o.h.b.t.a.p.EnclaveErrorAcceptanceTest#whenEnclaveIsDisconnectedGetReceiptReturnsInternalError` which validates an "Internal Error" proper json-rpc response. I changed the first two tests to be consistent with the later one.

Signed-off-by: Diego López León <[email protected]>
Signed-off-by: Diego López León <[email protected]>
@diega
Extract json-rpc HTTP authentication to a handler [hyperledger#535]
236abe4 
Signed-off-by: Diego López León <[email protected]>
Signed-off-by: Diego López León <[email protected]>
@diega
Move authentication json-rpc HTTP handler first in the execution pipe…
9038a64 
…line [hyperledger#535]

Signed-off-by: Diego López León <[email protected]>
Signed-off-by: Diego López León <[email protected]>
@diega
Replace TimeoutHandler in GraphQLHttpService with Vert.x's impl [hype…
933963f 
…rledger#535]

Signed-off-by: Diego López León <[email protected]>
Signed-off-by: Diego López León <[email protected]>
@diega
Extract json-rpc HTTP parser to a handler [hyperledger#535]
a34f2ff 
Signed-off-by: Diego López León <[email protected]>
Signed-off-by: Diego López León <[email protected]>
@diega
Refactor json-rpc HTTP handler [hyperledger#535]
7fe18c4 
Signed-off-by: Diego López León <[email protected]>
Signed-off-by: Diego López León <[email protected]>
@diega
Refactor json-rpc WS handler [hyperledger#535]
e2645ba 
Signed-off-by: Diego López León <[email protected]>
Signed-off-by: Diego López León <[email protected]>
@diega
Add json-rpc IPC support [hyperledger#535]
70a0196 
Signed-off-by: Diego López León <[email protected]>
Signed-off-by: Diego López León <[email protected]>
@sonarcloud
Copy link

sonarcloud bot commented Apr 26, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 6 Code Smells

80.5% 80.5% Coverage
0.0% 0.0% Duplication

mark-terry
mark-terry approved these changes Apr 26, 2022
View reviewed changes
Copy link
Contributor

@mark-terry mark-terry left a comment

Choose a reason for hiding this comment

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

LGTM.

@hyperledger hyperledger deleted a comment from lgtm-com bot Apr 26, 2022
@hyperledger hyperledger deleted a comment from lgtm-com bot Apr 26, 2022
@lgtm-com
Copy link

lgtm-com bot commented Apr 26, 2022

This pull request fixes 2 alerts when merging 70a0196 into bf349a4 - view on LGTM.com

fixed alerts:

  • 2 for Potential output resource leak

@hyperledger hyperledger deleted a comment from lgtm-com bot Apr 26, 2022
@macfarla macfarla merged commit 2884582 into hyperledger:main Apr 27, 2022
@diega diega deleted the jsonrpc_ipc branch April 27, 2022 01:06
@diega diega added the mainnet label May 29, 2022
@garyschulte garyschulte mentioned this pull request Jul 8, 2022
Closed
@non-fungible-nelson
Copy link
Contributor

Hi @diega - can you check out #3959 ? Please ping me on Discord if you have questions or ideas on how to resolve this.

@diega
Copy link
Contributor Author

diega commented Jul 12, 2022

Sure @matt-nelson-csi, thank you for the heads up

@diega diega mentioned this pull request Jul 13, 2022
Merged
2 tasks
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
@diega @eum602
Add IPC transport for JSON-RPC APIs (hyperledger#3695)
7fd0b78 
* Fix json-rpc HTTP tests [hyperledger#535]

The `o.h.b.e.a.j.JsonRpcHttpServiceTest#exceptionallyHandleJsonSingleRequest` and `o.h.b.e.a.j.JsonRpcHttpServiceTest#exceptionallyHandleJsonBatchRequest` tests were throwing ClassCastException in `o.h.b.e.a.j.JsonRpcHttpService#validateMethodAvailability` which wasn't ever catched, returning status 500 by default, but that wasn't the use case aimed to test. Another test running an exceptional method is `o.h.b.t.a.p.EnclaveErrorAcceptanceTest#whenEnclaveIsDisconnectedGetReceiptReturnsInternalError` which validates an "Internal Error" proper json-rpc response. I changed the first two tests to be consistent with the later one.

* Extract json-rpc HTTP authentication to a handler [hyperledger#535]

* Replace TimeoutHandler in GraphQLHttpService with Vert.x's impl [hyperledger#535]

* Extract json-rpc HTTP parser to a handler [hyperledger#535]

* Refactor json-rpc WS handler [hyperledger#535]

* Add json-rpc IPC support [hyperledger#535]

Signed-off-by: Diego López León <[email protected]>
Signed-off-by: Diego López León <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@macfarla macfarla macfarla left review comments

@atoulme atoulme atoulme approved these changes

@shemnon shemnon shemnon approved these changes

@antonydenyer antonydenyer antonydenyer approved these changes

@mark-terry mark-terry mark-terry approved these changes

Assignees
No one assigned
Labels
mainnet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support IPC transport for JSON-RPC APIs
8 participants
@diega @macfarla @non-fungible-nelson @atoulme @shemnon @antonydenyer @mark-terry @alexandratran