-
Notifications
You must be signed in to change notification settings - Fork 995
Fix IPC JSON-RPC concurrent request handling using streaming parser #9496
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
Conversation
135c01d to
09a3214
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
jflo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see performance notes, and CI is not currently passing. Once addressed, this is good to go!
| public static Handler<Buffer> ipcHandler( | ||
| final BiConsumer<JsonObject, JsonArray> onParsed, final Runnable onError) { | ||
| final JsonFactory jsonFactory = new JsonFactory(); | ||
| final ObjectMapper objectMapper = new ObjectMapper(jsonFactory); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JsonFactory and ObjectMappers are pretty heavyweight, these will get re-created on every ipc connection. Suggest making it a static member of the class or some other means of reuse. It is thread safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved them as static members of the class 👍
ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/ipc/JsonRpcIpcService.java
Outdated
Show resolved
Hide resolved
4ee336a to
59a81ba
Compare
Signed-off-by: Mael Regnery <mael@mqli.fr>
59a81ba to
8421e59
Compare
Signed-off-by: Mael Regnery <mael@mqli.fr> Signed-off-by: stefan.pingel@consensys.net <stefan.pingel@consensys.net>
Description
Fixes issue where concurrent JSON-RPC requests sent to the IPC socket fail with parse errors when they arrive in the same buffer as concatenated JSON objects.
Problem
When multiple requests arrive simultaneously at the IPC socket, they can be concatenated in a single buffer:
The current implementation tries to parse this as either a single
JsonObjectorJsonArray, which fails because concatenated JSON objects are neither.Solution
Replaced manual JSON parsing with Jackson's streaming parser (
JsonParser), which can naturally handle multiple JSON objects in a stream - similar to Go'sjson.Decoderused in go-ethereum.Key changes:
JsonRpcParserHandler.ipcJsonParser()using Jackson's streaming APIJsonRpcIpcService.javafrom ~300 to ~170 linesconcatenatedJsonRequestsShouldBeHandledIndependently()Benefits
Testing
All 6 IPC tests pass:
Files Changed
JsonRpcParserHandler.java- Added streaming JSON parserJsonRpcIpcService.java- Simplified to use streaming parserJsonRpcIpcServiceTest.java- Added concurrent request testRelated
This addresses the root cause discovered during investigation of why concurrent IPC requests fail while sequential requests succeed. Fixes #9495