Fix channel leakage when broker sends consumer cancel notifications #119

Closed
wants to merge 3 commits into
from

Projects

None yet

2 participants

@danielwhite
Contributor

The simplest way that this can occur is when deleting the queue (i.e. IAdvancedBus.QueueDelete).

What ends up happening is that the transient queue is removed from the ConsumerFactory, the underlying resources are never cleaned up and so the channels end up building up until the connection is lost.

Two potential issues:

  • PersistentConsumer currently isn't included in this since it seems as though it should handle cancellation differently.
  • The disposal of the model is a little funky. I'm not sure if the underlying cause is the same bug I've seen reported before: http://lists.rabbitmq.com/pipermail/rabbitmq-discuss/2012-October/023410.html. By disposing via the queue the model seems to get disposed successfully when connected to RabbitMQ.
danielwhite added some commits Nov 6, 2013
@danielwhite danielwhite Provide means to pass in an alternative queue for consumer tests d176279
@danielwhite danielwhite Extend MockBuilder to allow for consumer cancellation
This allows us to test the behaviour of a channel when RabbitMQ cancels
the consumer.
6cba50b
@danielwhite danielwhite Ensure transient consumer is disposed when the queue is deleted
Previously, the underlying factory would simply remove it from its list,
upon the receipt of 'basic.cancel' from the server.  Unfortunately, it
would not request for resources to be cleaned up and would end up leaving
consumers (and channels by extension) open indefinitely until the
underlying connection was lost.

It is yet to be determined if this should apply to PersistentConsumer, but
since it does very little when cancel occurs, it doesn't seem to apply.
0b36c1f
@mikehadlow
Contributor

Daniel, thanks for doing this, excellent work as usual. I want to test it thoroughly before pulling into master. How do you do integration test? By that I mean, how can you get RabbitMQ to send basic.cancel to a consumer?

@mikehadlow
Contributor

I guess if a queue that a persistent consumer is consuming from is deleted, then the correct behavior is for the channel to be closed and the consumer disposed.

@danielwhite
Contributor

Looks like I missed the integration tests before. I'll add them and update the pull request. At present, I'm triggering the behaviour by calling QueueDelete on the same bus that established the consumer.

As for the persistent consumers, my main concern is that the delete could come from numerous places. At least for transient consumers (i.e. exclusive queues), you can be sure that something on your connection triggered the delete. I might be missing something, but it appears that there isn't a means to determine if or when a consumer has been cancelled.

@mikehadlow
Contributor

I've taken your ideas and done my own implementation now. I think I'm going to take the opinionated stance that a deleted queue is a deleted queue, and EasyNetQ will not attempt to re-create it. Any consumers consuming from it will be closed and their channels shut down.

It turns out that implementing cancellation for this scenario also makes it easy to implement user cancellation, so I'll be releasing that at the same time, probably later today when I'm happy I haven't broken anything obvious.

Thanks so much for raising this issue and and your excellent work on the pull request, which I stole from shamelessly. I'm just sorry I took so long to pay it any attention.

@danielwhite
Contributor

I'm a happy man if the problem ends up solved. :)

@mikehadlow
Contributor

Closing. Thanks again.

@mikehadlow mikehadlow closed this Nov 6, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment