Deadlock in Inflight.NextImmediate when a publisher races the resend loop - #174
Open
goingforstudying-ctrl wants to merge 1 commit into
Open
goingforstudying-ctrl wants to merge 1 commit into
goingforstudying-ctrl wants to merge 1 commit into
Conversation
NextImmediate took RLock and then called GetAll, which takes RLock again. Once a writer queued behind the first read lock, Go blocks new readers, so the second RLock waited on itself and the client's whole packet loop hung. Split out getAllNoLock so NextImmediate only locks once. The quota helpers did load-then-add, which is not atomic. Publishers writing to the same subscriber run concurrently, so the send quota could drift below zero or past the max. Use CAS loops instead. GetAll sorted by uint16(Created), but Created is an int64 unix timestamp. The truncation wraps every ~18h and scrambles resend order on reconnect; compare the int64s directly.
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
1 participant
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.
Was load-testing a broker with a bunch of qos1 publishers against one slow subscriber and the subscriber's read loop wedged solid. Goroutine dump showed the client stuck in
Inflight.NextImmediateand a publisher stuck inInflight.Set, neither making progress.Turns out
NextImmediatetakesRLockand then callsGetAll, which takesRLockagain. Go's RWMutex blocks new readers as soon as a writer is queued, so if a publisher'sSetlands between those two read locks the goroutine holds the first lock forever and everything behind it (the whole per-client packet loop inprocessPacket, which hits the resend block at server.go:714 whenever inflight is non-empty and send quota is free) just hangs.While staring at that code two more things looked off:
if Load() > 0 { Add(-1) }), which isn't atomic. Publishers targeting the same subscriber run on different goroutines, so the send quota can drift below zero or above the max. A stress test with 32 goroutines pushed it to 65 with a max of 64.GetAllsorts byuint16(Created), butCreatedis an int64 unix timestamp. The truncation wraps every ~18 hours, so resend-on-reconnect ordering scrambles across the boundary. Demo: packets created at 65534..65537 come back as 65536, 65537, 65534, 65535.Changes, all in
mqtt/inflight.go:GetAllinto a locking wrapper plusgetAllNoLock;NextImmediateuses the no-lock variant under its own RLock, so it only locks onceAdded regression tests for all three in
mqtt/inflight_test.go: a deadlock repro that wedges immediately on the old code (hits the 10s timeout) and finishes in ~0.2s now, a quota storm that overshoots on the old code (65 > 64) and stays bounded now, and a sort test straddling the uint16 wrap boundary.go test ./mqtt/ -race -count=1passes (full package, not just the new tests).Not 100% sure CAS loops are the right call for the quota counters vs just taking the existing mutex — CAS keeps those hot paths lock-free, but I can redo it the other way if you'd rather keep it simple.