Take reject into account when sending confirms and vice-versa. #1719
Conversation
Before bf531fd rejects were not collected like confirms and extracted from unconfirmed. When adding the feature the important detail was missed: if unconfirmed dtree is empty, confirms will be sent as multiple confirming all messages up to latest. If there are rejects recorded, the channel can send multiple confirm and then reject right after with a lower message ID, which makes clients fail. Reported in php-amqplib/php-amqplib#597
|
Thanks @hairyhum! I will review this today. |
|
I'm having trouble reproducing the issue that @csernazs reports in php-amqplib/php-amqplib#597. However, that user can reproduce this issue and reports that these changes did not fix it. I'm going to continue investigating today. |
The cutoff value should be a number, corresponding to the minimal uncommitted or rejected message (or confirmed if sending rejects). erlang:min compares term values and will not traverse a list of NegativeMsgSeqNos.
|
There was a typo-like error. Instead of finding the smallest value in the list with additional value, it was comparing the list with that value. Oops. |
|
@csernazs reports that this PR fixes php-amqplib/php-amqplib#597. I'll do a review today but I think we're good. Nice job @hairyhum |
|
TODO left in the code is not new. I still would like to create a test to reproduce the error though. |
|
We concluded that an "external" (issue-specific) test would be nice but since it's highly time-sensitive and requires a lot of iterations [to prove an absence of the problem] it should not be included into the integration suite. Perhaps property-based testing of key functions would be optimal. Let's merge this. |
|
Added test in 3ae27d2 |
Take reject into account when sending confirms and vice-versa. (cherry picked from commit 631f101)
|
Backported to |
Take reject into account when sending confirms and vice-versa. (cherry picked from commit 631f101)
Before bf531fd rejects were not
collected like confirms and extracted from unconfirmed.
When adding the feature the important detail was missed:
if unconfirmed dtree is empty, confirms will be sent as multiple
confirming all messages up to latest.
If there are rejects recorded, the channel can send multiple confirm
and then reject right after with a lower message ID, which makes clients
fail.
Reported in php-amqplib/php-amqplib#597
TODO:
Come up with a reliable test for this scenario. Meddling with the stats timer may be required.