search

ruby-amqp / bunny

Bunny is a popular, easy to use, mature Ruby client for RabbitMQ
Other
1.39k stars 303 forks source link

Filter out mismatching (e.g. arriving after timeout) queue.declare-ok responses #566

Closed michaelklishin closed 6 years ago

michaelklishin commented 6 years ago

When a belated response for a queue.declare arrives, it will be filtered out like in the Java client's continuation mechanism.

WIP because this can be refactored further and potentially extended to at least one more method (exchange.declare).

References #558.

sbonebrake commented 6 years ago

With this change I can no longe reproduce the issue in #558 which is great and i'm happy with this approach. However, I see the java code references an issue with this method. https://github.com/rabbitmq/rabbitmq-java-client/blob/master/src/main/java/com/rabbitmq/client/impl/AMQChannel.java#L536 https://github.com/rabbitmq/rabbitmq-server/issues/710 I'm not certain how big of an issue that is, but something to consider.

michaelklishin commented 6 years ago

@sbonebrake the server will only strip non-printable characters as it makes it next to impossible to delete such queues using CLI tools or management UI.

I am surprised that https://github.com/rabbitmq/rabbitmq-server/issues/710 was mentioned there at all as it's very much an edge case. @acogoluegnes do you think it's a major concern?

acogoluegnes commented 6 years ago

I don't think it's a major problem, as the whole thing is a best-effort approach. We could check the name of the queue, we would have to strip the name the same way in both the client and the server.

I think it's reasonable to check the name matches in the request and the response by applying the same stripping logic as the server. This doesn't apply to server-generated queue names though :)

michaelklishin commented 6 years ago

So this cannot be extended to exchange.declare without a protocol extension because exchange.declare-ok doesn't carry a name (exchanges cannot be server-named). Let's stop at this and see what other methods may be worth safeguarding the same way.

hosamaly commented 2 years ago

This change breaks code that was using a Symbol for the queue name or the exchange name. If this is considered a good thing, it probably deserves to be mentioned on the changelog.

michaelklishin commented 2 years ago

@hosamaly you are the first person to notice this in over three years. No docs that I recall use symbols for topology names. I consider this to be a "feature" that worked by accident, and a mitigation should be pretty straightforward.