sec: drop BroadcastMu to remove head-of-line blocking on slow subscribers - #22
Merged
Conversation
…bers Closes #12. Broadcast held BroadcastMu for the entire iteration over the subscriber set. Under PolicyDisconnect's 10s writeDeadline, a single TCP-paused subscriber wedged every publisher to that channel for the duration. An attacker who could open a socket and stop ACKing-down-the-stack effectively held the channel hostage. Drop the mutex. Per-subscriber ordering — the only guarantee wirefan's protocol promises — is preserved by Go's channel-send ordering inside each Conn's send chan: messages arrive at the subscriber in the order Send was called. Cross-subscriber ordering ("every subscriber sees publish A before publish B") was never a documented contract. Concurrent broadcasts can now interleave their Send calls; each subscriber still sees its own messages FIFO. The Channel.BroadcastMu field is removed entirely (internal/* is not part of any external API).
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.
Summary
Closes #12.
BroadcastheldBroadcastMufor the entire iteration over the subscriber set. UnderPolicyDisconnect's 10swriteDeadline, a single TCP-paused subscriber wedged every publisher to that channel for up to 10 seconds. An attacker who could open one socket and stop ACKing-down-the-stack effectively held the channel hostage.Drop the mutex. Per-subscriber FIFO ordering — the only guarantee the protocol promises — is preserved by Go's chan-send ordering inside each Conn's
sendchan: messages arrive at the subscriber in the orderSendwas called. Cross-subscriber ordering ("every subscriber sees publish A before publish B") was never a documented contract.Concurrent broadcasts can now interleave their
Sendcalls. Each subscriber still sees its own messages FIFO.What changed
internal/registry/registry.go: removedChannel.BroadcastMufield.internal/hub/channel.go:Broadcastno longer locksBroadcastMu. Updated comment to explain the ordering guarantee.internal/*is not part of any external API; the field rename is safe.Test plan
go build ./...clean.go test ./internal/...passes (pre-existing CGO sqlite failure is environmental, unrelated).Conflict notes
Touches
internal/hub/channel.goandinternal/registry/registry.go. PR #20 (channel GC) also touches both files, with a newErrChannelDeletederror and aDeleted atomic.Boolfield. Conflicts are textually adjacent but logically independent — both can coexist. Whichever lands second will need a small textual rebase.