rabbitmq / rabbitmq-server

Open source RabbitMQ: core server and tier 1 (built-in) plugins
https://www.rabbitmq.com/
Other
12.16k stars 3.91k forks source link

AMQP 1.0 queue/link property equivalence requirement are not as strict as those of AMQP 0-9-1 #2585

Closed k-wall closed 3 weeks ago

k-wall commented 4 years ago

Currently when using the RabbitMQ AMQP 1.0 plugin to send or receive messages to a queue that already exists on the RabbitMQ server, the AMQP 1.0 caller needs to ensure that terminus durability indicated on the source/target of the Attach matches with the durability of the queue on the server. If this is not done, the plugin's behaviour is to End the AMQP session.

@end(23) [error=@error(29) [condition=:"amqp:precondition-failed", description="PRECONDITION_FAILED - inequivalent arg 'durable' for queue 'stocks' in vhost '/': received 'false' but current is 'true'"]]

In a complex messaging environment having to spread this durability knowledge about the system is undesirable and contrary to the Information Hiding principle.

The AMQP 1.0 protocol, "2.6.3 Establishing Or Resuming A Link", allows for more flexible behaviour:

"When a link is established, it is possible for an endpoint not to have all the capabilities necessary to create the terminus exactly matching the expectations of the peer. If this happens, the endpoint MAY adjust the properties in order to succeed in creating the terminus. In this case the endpoint MUST report the actual properties of the terminus as created."

So I think in the case where the queue already exists but the durability is mismatched, the RabbitMQ AMQP 1.0 plugin ought to allow the Attach should succeed. The Attach response should report actual durability. If a client cares, it can examine the Attach response and ensure that is terminus durability needs are met and Detach if it doesn't like what it finds.

Would it be desirable/possible for the plugin to be changed to work in this way?

k-wall commented 4 years ago

I took a look at the implementation in ensure_target and ensure_source. My initial idea was to use an additional queue.declare in order to ascertain the durability of the queue, however, I believe this approach would fail as backing_channel would be already lost by the channel.close resulting in initial queue.declare's failure. Is there an alternative way to query the durability of a queue?

michaelklishin commented 4 years ago

This is because that's how AMQP 0-9-1 works. I disagree that "having to spread this durability knowledge about the system is undesirable" but the declaration can be avoided by this plugin one way or another.

A passive declare on a separate channel is one way to go about it. rabbit_amqqueue:lookup/1 or a similar (potentially currently non-existent but trivial to add) function is another. There is no way to modify the properties of an existing queue other than deleting and redeclaring it. I highly doubt this idea is as great as it sounds at first.

In any case, this plugin can avoid doing a re-declaration if the target queue already exists. This can help hide the mismatch in how to queue property mismatch is treated in the protocols and in the short term, one depends on the building block of the other.

michaelklishin commented 4 years ago

So in some cases where properties are of no particular importance or we don't have enough context, we can see if the target queue already exists and avoid redeclaring it. This would avoid the channel exception.

In some cases, it might matter a lot (e.g. in MQTT, QoS is an explicit client-provided property). Before protocol plugins, including this one, move to use dedicated channel-like abstractions 100% specific to those protocols, there is no acceptable way to avoid an exception. I don't know if such cases should be silently ignored. We'll have to revisit this when we get to such major protocol plugin refactoring (no ETA promises, could be 4.0).

k-wall commented 4 years ago

@michaelklishin thanks for the quick response, I'll take a look at the two suggestions and put together a PR.

michaelklishin commented 4 years ago

@k-wall I think we can only skip doing a queue.declare when the implied durability level is "not durable" or unknown. We cannot do it when the used has asked for a durable queue/etc. So the above idea would eliminate the exception in some cases but not all, and unless this plugin is entirely decoupled from AMQP 0-9-1 underpinnings, we should not consider any more workarounds. @kjnilsson @Vanlightly

michaelklishin commented 4 years ago

I like this idea because while it will work for some cases only, it's a meaningful usability improvement that can be applied to MQTTv3 as well. I expect a basic "does queue X exist" check to not have a meaningful throughput effect compared to what we do today. Sounds like we can use dirty (local) record reads for queue objects, too, in this particular case.

k-wall commented 4 years ago

@michaelklishin If I understand you right, you are suggesting that queue.declare step is skipped if the terminus field of the source/target is none (0). In my experience it is common for AMQP 1.0 client to leave this field at its default value.

I agree this would give a usability improvement for the outgoing link case (i.e. rabbit_amqp1_0_outgoing_link). It would mean that an outgoing link to a durable or non-durable queues could be formed with experiencing the error. In the event that the queue did not exist, the basic.consume will fail, closing the AMQP 0-91 channel and (assuming today's behaviour) the AMQP 1.0 session would die as a result.

For the incoming link case, the happy path looks good too. However, I don't think it works in the case where the queue does not exist. rabbit_amqp1_0_incoming_link must still validate that the target exists with asserting durabilty (rabbit_amqqueue:lookup/1?) before the attach response is returned.

michaelklishin commented 4 years ago

@k-wall we should only skip queue declaration if

ansd commented 3 weeks ago

Closing due to inactivity. If this issue persists in RabbitMQ 4.0 with Native AMQP 1.0 and this issue is still relevant, please re-open.