-
Notifications
You must be signed in to change notification settings - Fork 227
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: handle receipt modAck and lease extensions with exactly-once delivery correctly #1709
Conversation
…delivery if the receipt modAck fails
…y-once delivery enabled
message.modAckWithResponse(deadline).catch(() => { | ||
// In the case of a permanent failure (temporary failures are retried), | ||
// we need to stop trying to lease-manage the message. | ||
this.remove(message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be setting the response for the eventual ack as failed as well. This way, when the client acks the message, we do not have to issue a server call. If the modack fails with a permanent error, ack will also fail with a permanent error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maheshgattani Can you check the change I just made to validate that it meets the requirements? The ack/modAck/nack methods will only throw a failure if the error is permanent, so I think this covers it.
Two fixes in this one:
I also discovered that we were missing a bunch of unit tests around this stuff, so I added some more.
Fixes #1697