Improve behavior of topology recovery when channels have been closed

[teemumurtola]

Consider the following code fragment using the RabbitMQ.Client library:

var connection = ...; // create connection with connection recovery and topology recovery on
using (var channel = connection.CreateModel())
{
channel.QueueDeclare("MyQueue", exclusive: true);
channel.Close();
}
... // Create a separate channel and start a consumer on "MyQueue"

After this has executed, assume that the connection to RabbitMQ breaks and is recovered. What should happen?

At least in RabbitMQ.Client (tested on version 5.0.1, but the recovery code still looks about the same in current master), "MyQueue" is not created on recovery, but rather multiple exceptions get thrown. The first one is an "AlreadyClosed" exception, from trying to declare "MyQueue", followed by exceptions from the consumer not finding the queue.

The behavior is not intuitive. I would expect that either

1. MyQueue is created on reconnection, or
2. Recreation of MyQueue is not even attempted, avoiding the exceptions.

The reason for this seems to be that the AutorecoveringModel that is used to call QueueDeclare() is recorded in RecordedQueue, and then the topology recovery calls QueueDeclare() on it. But since that channel has already been disposed, this fails. Possible solutions could be to either

1. The recovery should work on channels that may not be the original ones (there can be some complications here; haven't thought about it that much),
2. Closed channels should get reopened for recovery and then closed again, or
3. Recorded entities for closed channels should get thrown away.

[michaelklishin]

If something hasn't been declared because it was attempted on a closed channel it won't be recovered. This seems perfectly intuitive to me.

[michaelklishin]

There is no difference between channels that were recovered and were not. Explicitly closed channels won't be recovered by design: the user told the library that they are supposed to be closed.

Not all entities declared on channel A must be considered to be deleted when the channel is closed. A substantial % of users would expect those entities to be recovered anyway (especially if they are durable).

Automatic connection recovery is opinionated by definition. The client offers an API for manual recovery. We won't be changing automatic recovery behavior which is several years old and has been used in multiple clients successfully without a lot of convincing on the mailing list.

[teemumurtola]

It is fine that explicitly closed channels would not be recovered. That was an option in my suggested solutions. But then ideally they should not be causing exceptions/errors during recovery.

You write that a substantial number of users would expect entities on a closed channels to be recovered, but that is exactly what is not happening because of the current behaviour. It does not matter whether they were declared durable or not.

[teemumurtola]

To clarify: I picked the original example with an exclusive queue, because there the behaviour is easiest to observe. In that case, the consumer receives messages from the queue until the connection breaks, but does not receive messages after reconnection because the queue is no longer there.

But the recovery behaviour does not depend on whether the queue is exclusive or not. If the queue were durable, the recovery would still throw an exception and report an error, but the consumer would continue receiving messages because the queue is there even when the client does not redeclare it. But if someone were to delete the queue during the connection break, it would not get recreated.

I accept that you do not want to change the logic. At minimum, it would be good to clearly document that the recovery only works on declarations that were done on channels that are still open. And that if the user wants to avoid recovery errors, then no channel that was used to declare an entity should be closed (unless those entities have been deleted explicitly or through autoDelete). It took me a long time to figure this out.

[teemumurtola]

Sorry, I now see that this is actually documented in the API guide, except for the behaviour that there will be exceptions for all declarations that were done on closed channels.

[teemumurtola]

Sorry again for confusion, which was probably caused by me not reading the documentation more thoroughly, and as a result, suggesting alternatives that would require changing the documented behaviour.

Now, as a concrete suggesting, could you consider just changing the topology recovery such that if the channel has already been closed (e.g., in RecordedQueue.Recover()), then the declare call is skipped. This should not have any effect visible on the API surface, but the library would no longer internally throw exceptions or log errors in such cases. That would make it clearer that the library works as expected, e.g., to developers running the code under debugger.