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 #423 resent msg not added to timeout queue #582

Conversation

hylkevds
Copy link
Collaborator

Messages that are re-sent were not added to the timeout queue, as a result, if the re-sent failed as well, the message was not resent again, instead remaining on the inflightWindow for ever. By re-adding it to the inflightTimeouts, the message is "kept alive" until sending succeeds.

inflightTimeouts.add(new InFlightPacket(notAckPacketId.packetId, FLIGHT_BEFORE_RESEND_MS));

The second commit is a slight performance optimisation, doing a get + null check instead of a contains + get

@hylkevds hylkevds changed the title Fix 423 resent msg not added to timeout queue Fix #423 resent msg not added to timeout queue Feb 24, 2021
@hylkevds hylkevds force-pushed the fix_423_resent_msg_not_added_to_timeoutQueue branch from bac9f8a to 696ab80 Compare May 9, 2021 16:37
@andsel
Copy link
Collaborator

andsel commented Jun 29, 2021

Hi @hylkevds thank's for this bugfix, please could you rebase to master and add tests as in #607

hylkevds added 2 commits June 29, 2021 20:06
Messages that are re-sent were not added to the timeout queue, as a
result, if the re-sent failed as well, the message was not resent again,
instead remaining on the inflightWindow for ever. By re-adding it to the
inflightTimeouts, the message is "kept alive" until sending succeeds.
Between the contains and get calls, another thread could steal the
value. This also saves one hash lookup.
@hylkevds hylkevds force-pushed the fix_423_resent_msg_not_added_to_timeoutQueue branch from 696ab80 to 00aab1f Compare June 29, 2021 18:10
@hylkevds
Copy link
Collaborator Author

I've rebased on master, and cherry-picked the commit from #607
Looks like it builds :)

@andsel andsel self-requested a review June 29, 2021 19:48
Copy link
Collaborator

@andsel andsel left a comment

Choose a reason for hiding this comment

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

LGTM

@andsel andsel merged commit 9c540fd into moquette-io:master Jun 29, 2021
@hylkevds hylkevds deleted the fix_423_resent_msg_not_added_to_timeoutQueue branch July 20, 2021 07:18
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.

2 participants