rebus-org / Rebus.RabbitMq

:bus: RabbitMQ transport for Rebus
https://mookid.dk/category/rebus
Other
63 stars 44 forks source link

Fixing queue TTL functionality #55

Closed ronnyek closed 4 years ago

ronnyek commented 5 years ago

Long ago, I submitted changes to allow queue ttl so we could auto expire queues that no longer had any subscribers (after a certain amount of time) https://github.com/rebus-org/Rebus.RabbitMq/pull/43

I've since done a bit of research, as I've noticed that queue's are always destroyed the moment subscribers are disconnected (regardless of any queue-ttl). I'm trying to determine definitively whether this is happening at the rabbitmq level (bug, or mishandling of queue declare properties) or perhaps something specific to Rebus.RabbitMQ.

Is it possible Rebus is deleting the queue external to the flag on the queue declare? Upon subscribers disconnecting? (Eg, should it be deleting queues with no subscriber on disconnect and that we just need to provide an additional check to see if any TTL was provided?)

According to rabbitmq admin, it looks like it does set autodelete and x-expires, and can even see the TTL values I specified, but don't see queue ever sticking around for the 30 minutes I specified.

mookid8000 commented 5 years ago

I've never built anything into Rebus' RabbitMQ transport that deletes queues, so I think it's unlikely that something like that is happening.

Have you tried reproducing the behaviour without Rebus in the mix? Could be fun to see what would happen if you just coded it with raw RabbitMQ driver stuff.... 🤔

jarikp commented 4 years ago

Hi, I think I've found what is causing this problem. See suggested pull request.

ronnyek commented 4 years ago

I saw tests there... have you actually tested that queue creation happens, exists for ttl after all clients disconnect and ultimately delete? (I am gonna look at the unit test changes you included in your PR shortly)

I noticed problems with my original contributions... but in researching a fix to them, it almost seemed like the behavior in rabbitmq is indeterminate and not documented well enough. I was trying to make sure I could set up queue's the way I understood they needed to be via rabbitmq client directly, and couldn't seem to get that even to work the way I expected.

Also, in testing, I could see that specifying what I believe to be the correct values in the queue creation parameters, it looked to be creating the queue in admin showing it had a ttl etc.. but never seemed to behave as expected. As soon as I'd disconnect the last subscriber, the queue would be immediately deleted. If I omitted the autodelete and just left ttl, it'd never delete the queue.

jarikp commented 4 years ago

Hi @ronnyek

I tried to unit tests, but some of this behavior is not deterministic, meaning that RMQ does not guaranty when exactly it will delete expired queues. Indeed, this is a bit confusing. As I read the docs for RMQ, there are following properties (https://www.rabbitmq.com/queues.html) related to this topic:

The auto-delete option is set explicitly when the queue is declared, whilst TTL is set through a client property x-expires argument.

The RMQ docs says that "No guarantee is given as to how promptly the queue will be removed after the expiration period has elapsed", this is why unit testing is a bit ugly and not reliable here.

But the problem which my PR address is not because of problems with RMQ, but due to a mistake in the code: if auto-delete is "false", then the property x-expires is never propagated to RMQ when a queue is created, which explains what you see in your application. A simple way to check this is to look for an EXP badge in the queue features: image the tooltip indicates the TTL in MS.

Hopefully, @mookid8000 will review and advise on how we can test this behavior in a proper way using the Rebus unit testing framework.

ronnyek commented 4 years ago

Oh yeah, there definitely was assumptions made about the fact that those two props were tied to one another. And it looks like the tests (without running them) do attempt to validate the new understanding.

If this does in-fact work, I can't wait until this gets accepted... I've on and off (without being able to contribute a ton of time to it) to get the logic just right and test.

jarikp commented 4 years ago

then you can simple rewrite your code like this: .InputQueueOptions(o => o.SetAutoDelete(false, 1000).... into this .InputQueueOptions(o => o.SetAutoDelete(false).AddArgument("x-expires", 1000)...

it will work as well, you don't have to wait for an update from Rebus.RabbitMQ