Zero freed slots before pooling/reslicing in clearLocked and delRequest#79
Merged
Conversation
0f4a98e to
e5a2185
Compare
Two GC-retention nits where a freed slice slot kept a stale reference live, inconsistent with the deliberate reference-clearing elsewhere in the same files. clearLocked reset the outbound queue with rq.wsQueue = rq.wsQueue[:0], leaving the previous wire.WsMsg values (including Data, which carries full Inner/Replace/Append HTML payloads) live in the backing array of a Request returned to the pool. A pooled Request can sit idle indefinitely, pinning those payloads until a future reuse overwrites each slot. This is inconsistent with the adjacent clear(rq.todoDirt) and clear(rq.elems) (both commented as releasing references before pooling) and with drainTailScript, which zeroes its freed slots. Add clear(rq.wsQueue) before the reslice. delRequest swap-removed an entry but never nilled the vacated tail slot, so the backing array retained an otherwise-recyclable *Request pointer (the duplicated one after a swap, or the original when removing the last element) for the session's lifetime. Nil the freed slot before reslicing. Neither is a correctness bug (all iterators use the resliced length), purely retention. Add internal tests asserting the freed backing-array slots are zeroed.
e5a2185 to
64ada8a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Defects (second-pass per-package review)
Two low-severity GC-retention nits where a freed slice slot kept a stale reference live, each inconsistent with the deliberate reference-clearing already present in the same file.
1.
clearLockedpools a Request without releasingwsQueuepayloads (request.go)clearLockedreset the outbound queue withrq.wsQueue = rq.wsQueue[:0], leaving the previouswire.WsMsgvalues — includingData, which carries fullwhat.Inner/Replace/AppendHTML payloads — live in the backing array of aRequestreturned tosync.Pool. A pooled Request can sit idle indefinitely, pinning those payloads until a future reuse overwrites each slot. This is inconsistent with the adjacentclear(rq.todoDirt)andclear(rq.elems)(both commented as releasing references before pooling) and withdrainTailScript, which zeroes its freed slots.2.
delRequestleaves a stale*Requestin the backing array (session.go)The swap-remove copied
requests[l-1]into the found index then resliced to[:l-1], but never nilled the vacated tail slot — so the backing array retained an otherwise-recyclable*Request(the duplicated one after a swap, or the original when removing the last element) for the session's lifetime.Fix
clear(rq.wsQueue)before the[:0]reslice (still undermuQueue).sess.requests[l-1] = nilbefore reslicing.Neither is a correctness bug — all iterators use the resliced length, so the stale entries are never observed — purely retention/consistency.
Tests
slotclear_test.go(internalpackage jaws) asserts the freed backing-array slots are zeroed:TestClearLockedZeroesWsQueuequeues messages with payloads, runsclearLocked, and checkswsQueue[:cap]is all zero;TestDelRequestNilsVacatedSlotcovers both the swap-remove and last-element paths. Both fail on the unpatched code.Verification
go vet,gofumpt -l,staticcheck,go build,go test -race ./..., andgo test -tags debug -race ./...all pass.