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
[FIXED] Possible race causing ack'ed message to be redelivered #1139
Conversation
During redelivery, a go routine gathers all pending messages under a lock. However, the actual send of messages is done one by one and by then, one of the message in the list of messages to redeliver may have been acknowledged. So during the send, under the sub's lock, we need to check that if a message marked as redelivered is no longer in the pending list, it should not be sent to the client. Also fixed an unrelated data race that was just found running tests. Resolves #1138 Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
server/server.go
Outdated
@@ -3721,23 +3730,28 @@ func (s *StanServer) performAckExpirationRedelivery(sub *subState, isStartup boo | |||
// otherwise this could cause a message to be redelivered to multiple members. | |||
if !isClustered && qs != nil && !isStartup { | |||
qs.Lock() | |||
sub.Lock() | |||
skip := !sub.isMsgStillPending(m) |
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.
very minor nitpick, take it or leave it, but could use msgPending := sub.isMsgStillPending(m)
and avoid the !
s. It might read easier.
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.
Will do, and actually should not try to send if not pending.. so thanks!
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.
LGTM.
Used code review recommendation and fixed the queue case. Added queue sub in new test that would have caught the bug. Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
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.
LGTM.
During redelivery, a go routine gathers all pending messages under
a lock. However, the actual send of messages is done one by one
and by then, one of the message in the list of messages to redeliver
may have been acknowledged. So during the send, under the sub's lock,
we need to check that if a message marked as redelivered is no longer
in the pending list, it should not be sent to the client.
Also fixed an unrelated data race that was just found running tests.
Resolves #1138
Signed-off-by: Ivan Kozlovic ivan@synadia.com