DO NOT MERGE Reply reject_publish when queue marks a message as duplicate

[noxdafox]

The rabbit_backing_queue behaviour defines a is_duplicate callback
to signal if the message about to be published is a
duplicate. Nevertheless, the broker logic does not take any action if
so.

This becomes problematic if the client enables publisher confirmation
as neither a confirm nor a reject is provided as response in case
a message gets dropped as duplicate.

This patch ensures the client receives a rejection if a queue marks
the delivered message as duplicate.

Signed-off-by: Matteo Cafasso [email protected]

Types of Changes

• Bugfix (non-breaking change which fixes issue #NNNN)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation (correction or otherwise)
• Cosmetics (whitespace, appearance)

Checklist

• I have read the CONTRIBUTING.md document
• I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
• All tests pass locally with my changes
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)
• Any dependent changes have been merged and published in related repositories

Further Comments

Few users reported issues when using the message deduplication plugin together with publisher confirmation.

It appears the broker does not deliver any notification to the publisher when a backing queue is_duplicate callback returns true. This causes clients to hang indefinitely waiting for a confirmation/rejection to come.

This patch tackles the issue making sure a nack is delivered in case a rabbit_backing_queue implementation provides message deduplication.

To reproduce the issue, you can install the deduplication plugin from the releases tab and follow the instruction provided in this issue.

[pivotal-issuemaster]

@noxdafox Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[michaelklishin]

Can we take a step back and explain why the current behavior has to change?

[noxdafox]

Uhm... there seems to be some issue with GitHub and this issue ticket as @michaelklishin comment seems to appear/disappear?

@michaelklishin I explain what the issue is in both the commit message and in the "further comments". More information is also available at the linked issue. Can you please help me understanding what shall I clarify more?

[michaelklishin]

@noxdafox under what circumstances did the client not receive a nack? The commit message doesn't explain that. Why is this a good idea to nack such messages? What kind of applications can be affected?

[michaelklishin]

So the context is in noxdafox/rabbitmq-message-deduplication#21. I'm not convinced this is what RabbitMQ users who don't use that plugin want.

[michaelklishin]

We should carefully assess how this affects the users who do not use the message deduplication plugin.

[michaelklishin]

The backing queue behavior docs do not suggest if duplicates should be rejected but it does mention that the caller might consider treating the message as "dropped".

rabbit_variable_queue:is_duplicate/2 always returns false and rabbit_priority_queue:is_duplicate/2 passes through to the next BQ in the chain (e.g. the variable one).

It gets a lot more interesting in the classic mirrored queue implementation. Mirrors can see a message more than once due to how the gm module (the "transport" and current "consensus" implementation) works and handlers failures.
A duplicate in that case is not a reason to unconditionally communicate a nack to the client. In fact,
the is_duplicate/2 implementation there lists several cases in which the "caller" may or may not have sufficient information to decide on when to send confirms.

So this would be a breaking and surprising change to those who use classic mirrored queues: they would get false negatives.

[noxdafox]

According to the rabbit_backing_queue behaviour, the is_duplicate callback shall signal the queue process that the message about to be published is actually a duplicate. In such case, the queue process will not publish the message in the queue.

Nevertheless, the logic does not take this scenario into account when replying to the client. This becomes problematic if a client enables publish confirmation as the client is left hanging indefinitely.

I did not take into account the mirrored queues implementation actually. I see how the raised concern is valid.
I'll see if we can find a better way to tackle this. We don't want to send ack/nack back if the duplicate is the result of an internal duplication. Nevertheless, we don't want to let the client hang if a message is not enqueued because of some policy applied by the user.

[michaelklishin]

We'd welcome a different PR for 3.8 (master) that would make it possible to address noxdafox/rabbitmq-message-deduplication#21 without breaking changes.
I can't immediately think of a way to do this for mirrored queues but perhaps with quorum queues, when they are merged, there can be a way.

[michaelklishin]

@noxdafox I can see why the current behavior in amqqueue_process is problematic for your plugin.

As @kjnilsson and I concluded in a 1-on-1 conversation, the callback was originally added to introduce some of the idiosyncrasies and flaws in the gm module and thus classic mirrored queues, not to actually let plugins deduplicate :(

[kjnilsson]

Sadly the dynamic nature of RabbitMQ queues mean that many of the abstractions aren't as intuitive as they should be.

Could we extend the is_duplicate callback with a return value that tells the queue process to reject the message and not just drop it silently? like true | {true, drop} | {true, reject} | false where true is backwards compatible with {true, drop}

[noxdafox]

I see, the behaviour documentation did not mention the callback was for internal use only.

May I suggest to change the callback interface or add a new one? This would still enable the Use Case without the need of affecting mirrored queues implementation.

For example, the is_duplicate callback could return a tuple {bool, symbol} where the symbol could instruct the process what to do. Something like {true, ignore} or {true, reject}.

The change would be minimal within the broker logic but I am not sure how flexible we can be when it comes to change behaviour APIs.

EDIT: @kjnilsson was faster :)

[michaelklishin]

@noxdafox we can make any internal interface changes for 3.8.0. As long as on disk data and client protocol are not affected, we'd consider it ;)

[noxdafox]

I will start by providing a RFC PR changing the is_duplicate behaviour and the related logic.

We can later discuss whether to introduce nack for duplicated messages and what are the implications.