Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix loss of message after invalidating previous message in queue with single broadcast message #263

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pstibrany
Copy link

@pstibrany pstibrany commented Jul 14, 2022

We have found a condition where message can get overwritten in the TransmitLimitedQueue. This PR fixes it.

Message can be lost in scenario where queue has a single message in it, and then another message, which invalidates previous message, is added to the queue. During invalidation the old message is removed and the queue is temporarily empty. At that point internal idGen value was reset to 0. However new message is then added to the queue with id computed from previous value of idGen.

If later more messages are added to the queue, new message can overwrite existing message in the queue, because of conflicting id and exact same length. See the unit test for exact scenario we have observed in our system.

Simplest fix seems to be to avoid resetting the idGen when queue is empty. Alternative fix may adjust id of inserted message if idGen was reset during invalidation of other messages in (*TransmitLimitedQueue).queueBroadcast, or pass a flag to (*TransmitLimitedQueue).deleteItem to indicate if reset of idGen should be done or not.

@hashicorp-cla
Copy link

hashicorp-cla commented Jul 14, 2022

CLA assistant check
All committers have signed the CLA.

Copy link

@pracucci pracucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was part of this investigation and I confirm that avoiding resetting the idGen solves the issue. Since the queueBroadcast() already deals with the wrap around, I don't see any need to reset idGen to 0 to keep it low.

@pracucci
Copy link

A clarification: the id is used for comparsion by limitedBroadcast.Less(). As the comment says:

// If !a.Less(b) && !b.Less(a), we treat this to mean a == b (i.e. we can only
// hold one of either a or b in the tree).

Given two items, if none of the two is less than the other, then they're considered equal by the btree implementation, and the btree.ReplaceOrInsert() (called by TransmitLimitedQueue.addItem()) will replace the previous one (because considered equal).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants