Generate id after deleting invalidated broadcasts
#322
Conversation
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
Under certain conditions, a broadcast in a TransmitLimitedQueue can be erroneously dropped (without invoking Finished()) when a new broadcast which does not invalidate it is added to the queue. The queued broadcasts are stored in a B-tree, sorted by priority. Broadcasts are sorted lexicographically by (transmits, -length, -id) such that longer messages are prioritized over shorter ones, and newer messages take priority over older ones of the same length. The B-tree implementation requires a strict ordering of broadcasts. If two items have the same priority (!a.Less(b) && !b.Less(a)) they are considered equal. When a new item is inserted into the tree which is equal to an existing item, the existing item is replaced with it. TransmitLimitedQueue tries to ensure that no broadcast in the tree is equal to any other by giving each broadcast a distinct id value: the number of broadcasts enqueued since the queue was last emptied. But it sometimes assigns an id value to a broadcast which collides with the id of another broadcast already in the queue! The trouble arises when adding a broadcast to the queue which invalidates all the existing broadcasts. The id of the new broadcast is generated first, before the invalidated broadcasts are removed from the queue. (The new broadcast is inserted into the tree last, after removing the invalidated ones.) When all broadcasts are invalidated, the id generator state is reset due to the queue being (transiently) empty. The end result is a queue containing a single broadcast with an id of N>1 and an id generator state such that the next id generated will be 1. The Nth broadcast added to the queue after this will be assigned id N -- id collision! If the message lengths happen to be equal, the Nth broadcast will clobber the previous broadcast in the queue with the colliding id when inserted into the tree. Fix the erroneous loss of enqueued broadcasts by generating the id for the new broadcast after invalidating any existing ones, rather than before.
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.
Under certain conditions, a broadcast in a
TransmitLimitedQueuecan be erroneously dropped (without invokingFinished()) when a new broadcast which does not invalidate it is added to the queue. The queued broadcasts are stored in a B-tree, sorted by priority. Broadcasts are sorted lexicographically by(transmits, -length, -id)such that longer messages are prioritized over shorter ones, and newer messages take priority over older ones of the same length.The B-tree implementation requires a strict ordering of broadcasts. If two items have the same priority (
!a.Less(b) && !b.Less(a)) they are considered equal. When a new item is inserted into the tree which is equal to an existing item, the existing item is replaced with it.TransmitLimitedQueuetries to ensure that no broadcast in the tree is equal to any other by giving each broadcast a distinctidvalue: the number of broadcasts enqueued since the queue was last emptied. But it sometimes assigns anidvalue to a broadcast which collides with theidof another broadcast already in the queue!The trouble arises when adding a broadcast to the queue which invalidates all the existing broadcasts. The
idof the new broadcast is generated first, before the invalidated broadcasts are removed from the queue. (The new broadcast is inserted into the tree last, after removing the invalidated ones.) When all broadcasts are invalidated, the id generator state is reset due to the queue being (transiently) empty. The end result is a queue containing a single broadcast with anidof N>1 and an id generator state such that the nextidgenerated will be 1. The Nth broadcast added to the queue after this will be assignedidN --idcollision! If the message lengths happen to be equal, the Nth broadcast will clobber the previous broadcast in the queue with the collidingidwhen inserted into the tree.Fix the erroneous loss of enqueued broadcasts by generating the
idfor the new broadcast after invalidating any existing ones, rather than before.