fix: dispose JsonDocument for batch JSON-RPC requests#10139
fix: dispose JsonDocument for batch JSON-RPC requests#10139Snezhkko wants to merge 2 commits intoNethermindEth:masterfrom
Conversation
Snezhkko
requested review from
LukaszRozmej,
benaadams and
smartprogrammer93
as code owners
LukaszRozmej
left a comment
LukaszRozmej
left a comment
There was a problem hiding this comment.
Can you check other potential problems with disposing?
| // Increments failure metric and logs the exception, then stops processing. | ||
| Metrics.JsonRpcRequestDeserializationFailures++; | ||
| if (_logger.IsDebug) _logger.Debug($"Couldn't read request.{Environment.NewLine}{e}"); | ||
| yield break; |
| { | ||
| // Logs exception, then stop processing. | ||
| if (_logger.IsTrace) _logger.Trace($"Connection reset.{Environment.NewLine}{e}"); | ||
| yield break; |
| // Checks for deserialization failure and yields the result. | ||
| if (deserializationFailureResult.HasValue) | ||
| { | ||
| yield return deserializationFailureResult.Value; |
| { | ||
| if (_logger.IsWarn) _logger.Warn($"The batch size limit was exceeded. The requested batch size {collection.Count}, and the current config setting is JsonRpc.{nameof(_jsonRpcConfig.MaxBatchSize)} = {_jsonRpcConfig.MaxBatchSize}."); | ||
| JsonRpcErrorResponse? response = _jsonRpcService.GetErrorResponse(ErrorCodes.LimitExceeded, "Batch size limit exceeded"); | ||
| response.AddDisposable(() => jsonDocument.Dispose()); |
There was a problem hiding this comment.
What about collection here and in previous comments?
There was a problem hiding this comment.
What about
collectionhere and in previous comments?
Added disposal in the generic catch so we don’t leak JsonDocument / collection when DeserializeObjectOrArray throws, and removed the extra AddDisposable(() => collection.Dispose()) from the batch path — the collection is already disposed in IterateRequest’s finally, now both it and JsonDocument are disposed exactly once on every path.
|
I don't think this will work correctly |
There was a problem hiding this comment.
Pull request overview
This PR fixes a resource leak in batch JSON-RPC request handling by ensuring JsonDocument instances are properly disposed. Previously, only single requests and error paths registered JsonDocument disposal, while successful batch requests leaked the pooled document buffers, causing increased GC pressure under sustained batch load.
Changes:
- Added JsonDocument disposal registration for batch requests via JsonRpcBatchResult
- Added proper cleanup of both jsonDocument and collection in the exception handler
|
Replaced by #10207 |
2 participants
Previously, successful batch JSON-RPC requests never disposed the JsonDocument instance created during parsing. Single requests and error/limit paths correctly attached JsonDocument.Dispose() via JsonRpcResponse.AddDisposable, but the normal batch path only registered disposal for the ArrayPoolList of requests. As a result, the pooled JsonDocument buffers were effectively leaked, increasing GC and memory pressure under sustained batch load. This change attaches JsonDocument.Dispose() to the JsonRpcBatchResult lifetime so that the document is released when the batch result is disposed