Repository navigation
Conversation
Validate a single complete request before identifying notifications on the WebSocket admission rejection path, and skip writes for valid notifications. Add transport regression coverage for notification errors, request IDs, and malformed input while the shared request gate is full. Signed-off-by: Arunim Shukla <[email protected]>
97169f0 to
49efc3f
Compare
brbrr
left a comment
brbrr
left a comment
There was a problem hiding this comment.
Thank you for this fix.
Before we can merge it, though:
- There is a merge conflict that needs to be addressed.
- Lint error in tests
Also, see the small comments below.
731ba2e to
49efc3f
Compare
Signed-off-by: Arunim Shukla <[email protected]>
Check WebSocket cleanup errors, remove the redundant handler counter, and expand notification and malformed-input regression coverage to 12 cases. Validated with Go 1.27.0: regression tests, the JSON-RPC race suite, go vet, both lint configurations, and formatting checks pass. Signed-off-by: Arunim Shukla <[email protected]>
brbrr
left a comment
brbrr
left a comment
There was a problem hiding this comment.
one last thing is left - properly handle the case of batched notifications.
Recognise non-empty batches containing only sane requests without IDs before returning a WebSocket admission error. Preserve existing busy responses for mixed, empty and malformed batches. Expand the public WebSocket regression from 12 to 30 cases. The five new notification-batch cases fail on the previous PR head; all 30 pass with this fix. The JSON-RPC race suite, go vet, both lint configurations and formatting checks pass with Go 1.27.0. Signed-off-by: Arunim Shukla <[email protected]>
brbrr
left a comment
brbrr
left a comment
There was a problem hiding this comment.
Thank you, the notification-only batch case works now. Two issues remain:
-
The busy path does not apply the batch limits. The normal path rejects a batch when batches are disabled or when the batch is larger than
maxBatchElements. This branch skips both checks. Thus, when the gate is full, a notification-only batch that is too large gets no reply. When the gate is free, the same batch getsInvalidRequest. Please returnserverBusyResponsein these two cases. -
The busy path now decodes the full batch.
json.Unmarshalinto[]Requestdecodes and copies every element before the check. A 10,000-element batch now takes 3.2 ms and 4.5 MB to reject (before: 0.3 µs). This occurs when the node is already saturated. Please decode one element at a time with ajsontext.Decoder, and stop at the first element that has an ID or is not valid.
Respect DisableBatchRequests and WithMaxBatchElements before suppressing notification-only batches. Read one element at a time with jsontext.Decoder, stop at the first identified or invalid request, and validate the closing delimiter and EOF before returning no response. Use an aliased bytes.Buffer and the pooled request decoder rather than materialising []Request or allocating a string cache for each busy batch. Add batch-policy boundary cases and compare them with normal dispatch through public APIs. Three of 37 WebSocket cases fail on the previous PR head; all 37 pass with this correction. The full JSON-RPC race suite, vet, both lint configurations and formatting checks pass with Go 1.27.0 and golangci-lint 2.15.0. Public WebSocket benchmarks (100 iterations, three repeats): the 10,000-element first-ID case has median 4.06 ms / 4.30 MB before and 0.53 ms / 0.72 MB after, including transport costs. Notification-only and small batches are also benchmarked. Small batches retain about 0.3-0.4 KB of additional streaming-decoder overhead; no constant-time claim is made for batches containing only notifications. Signed-off-by: Arunim Shukla <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
When the WebSocket admission gate is full, a valid single-request notification receives a
-32004error withid: null. Rejection happens before normal dispatch, so notification response suppression is never reached.Decode one complete JSON value with the current JSON v2 decoder and validate the request shape before treating an absent ID as a notification. Return nil for those notifications and skip the WebSocket write. Requests with IDs and malformed input continue receiving busy errors. Batch admission behaviour is unchanged.
The regression test fills the gate through its public API and uses a real WebSocket connection. An identified probe sent after each notification checks that no unsolicited reply was emitted, without sleeps. Twelve cases cover notifications, unknown methods, invalid method arguments, trailing whitespace, string and large numeric IDs, explicit null IDs, invalid IDs, malformed request shapes, trailing garbage, and a second JSON value.
Review changes
main(765b182f3e017277403a87a0851517529254d582) and resolve the conflict usingjson.Unmarshal(body, &req, decodeOptions); the old decoder helper was removed upstream.busyResponsemeans no reply must be sent.conn.CloseNow()in test cleanup, fixing the reproducederrcheckfailure.Validation
Go 1.27.0; golangci-lint 2.15.0:
go test -short ./jsonrpc -run '^TestWebsocketGateNotificationSemantics$' -count=1 -v— all 12 cases pass with the fix.go test -race -short ./jsonrpc -count=1— passes.go vet ./jsonrpc— passes.golangci-lint run ./jsonrpc/...— zero issues.golangci-lint run --config=.golangci_diff.yaml --new-from-rev=origin/main ./jsonrpc/...— zero issues.golangci-lint fmt --diff ./jsonrpc/...— no formatting changes.git diff --check origin/main— passes.AI assistance: prepared and tested with Codex on behalf of @arunimshukla.