Conversation
There was a problem hiding this comment.
Pull request overview
This pull request implements chunk-based JSON parsing for incoming JSON-RPC requests to improve performance and handle large payloads that arrive in multiple network packets. Instead of reading all data at once with ReadToEndAsync, the code now processes data incrementally using ReadAsync in a loop, maintaining parser state between chunks.
Changes:
- Replaced
ReadToEndAsyncwith chunkedReadAsyncloop for incremental JSON parsing - Introduced
JsonReaderStateto preserve parser state across chunk boundaries - Refactored
ProcessAsyncmethod to handle partial JSON documents and resume parsing when more data arrives
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Nethermind/Nethermind.JsonRpc/JsonRpcProcessor.cs | Implements chunk-based JSON parsing with state management, replaces synchronous full-read pattern with asynchronous incremental processing |
| src/Nethermind/Nethermind.JsonRpc.Test/JsonRpcProcessorTests.cs | Adds test for processing multiple large requests arriving in chunks to verify the new parsing behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
benaadams
left a comment
There was a problem hiding this comment.
I didnt know STJ supported this; I love it :)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/Nethermind/Nethermind.JsonRpc/JsonRpcProcessor.cs:134
- This early exit check does not prevent the subsequent code from executing. When ProcessExit.IsCancellationRequested is true, the method yields an error response but continues to execute the rest of the method. This should either return after yielding or be wrapped in an else clause to prevent the reader from being processed unnecessarily.
public async IAsyncEnumerable<JsonRpcResult> ProcessAsync(PipeReader reader, JsonRpcContext context)
{
if (ProcessExit.IsCancellationRequested)
{
JsonRpcErrorResponse response = _jsonRpcService.GetErrorResponse(ErrorCodes.ResourceUnavailable, "Shutting down");
yield return JsonRpcResult.Single(RecordResponse(response, new RpcReport("Shutdown", 0, false)));
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/Nethermind/Nethermind.Core/Extensions/ReadOnlySequenceExtensions.cs:45
- There’s an extra closing brace here, which will break compilation (method/class braces become unbalanced). Remove the stray
}so the method and class close correctly.
// The entire sequence was trimmed chars
return sequence.Slice(sequence.End);
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/Nethermind/Nethermind.JsonRpc.Test/JsonRpcProcessorTests.cs
Outdated
Show resolved
Hide resolved
|
@LukaszRozmej I've opened a new pull request, #10356, to work on those changes. Once the pull request is ready, I'll request review from you. |
…ests (#10356) * Initial plan * Change test endpoint from Http to Ws for multiple requests test Co-authored-by: LukaszRozmej <12445221+LukaszRozmej@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: LukaszRozmej <12445221+LukaszRozmej@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/Nethermind/Nethermind.JsonRpc.Test/JsonRpcProcessorTests.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/Nethermind/Nethermind.JsonRpc.Test/JsonRpcProcessorTests.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private ArrayPoolList<JsonRpcRequest> DeserializeArray(JsonElement element) => | ||
| new(element.GetArrayLength(), element.EnumerateArray().Select(DeserializeObject)); | ||
|
|
||
| private static readonly JsonReaderOptions _jsonReaderOptions = new() { AllowMultipleValues = true }; |
There was a problem hiding this comment.
Allowing multiple top-level JSON values in the reader (AllowMultipleValues=true) combined with the HTTP pipeline only consuming the first JsonRpcResult (Startup.cs breaks after the first response) means trailing non-whitespace bytes (or even a second JSON document) can be silently ignored for HTTP requests. If HTTP is intended to accept exactly one JSON-RPC request (object or batch array), consider making this behavior endpoint-specific: for Http contexts, validate that the remaining bytes after the first parsed value are only whitespace once the body is complete, and return a single ParseError/InvalidRequest instead of yielding a successful response for the first value.
| private static readonly JsonReaderOptions _jsonReaderOptions = new() { AllowMultipleValues = true }; | |
| private static readonly JsonReaderOptions _jsonReaderOptions = new() { AllowMultipleValues = false }; |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Should replace #10191
Changes
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
RPC traffic.
Remarks
One thing which is kind of wrong is that we ignore trailing garbage:
Will result in parsing error in Geth and correct response from us.
I tried to make it work with looking into how to handle it.
But it also depends on the channel - through sockets we can get multiple JSON documents one by one. Which is not happening by http.
If they got chunked in any way we would need to try to wait for next chunk.
It all got very complicated.
What we could do:
{or[- which at least haves a chance to be proper json