The new code doesn't count the number of times we have been at a certain queue, it just records the fact that that we have been there. That's removing a bit too much information IMO, a counter would be useful, non-leaky, and easy to add.

I am not completely convinced this has not broken cycle detection for some more arcane types of cycles. I shall try to have a think about that.

The Java tests introduce new Channel APIs! That's an unexpected side effect of this bug 😄 I don't think Channel.queueBind(String queue, String exchange) is actually a great idea; it de-emphasises routing keys and thus might rather confuse beginners.

I agree, the number of times would be nice. The user who reported the problem mentionned it as well.

After reading the cycle detection code, I think it still works: it only looks for one entry for the current queue.

Yes, but cycle detection makes decisions based on the {reason, queue} pair, but the branch only keeps one header per queue. That smells dubious to me.

So for example if you have a message which (for some odd reason) keeps getting dead lettered to the same queue, but with alternating reasons of expired and rejected then this branch changes the behaviour - we used to not drop it (since we only drop messages on dead-letter cycles that are fully automatic, requiring no client intervention to keep them going) but on this branch we would drop it.

I'm not sure if I can get a change in behaviour the other way round (which would be much more dangerous of course). And that is a bit of a contrived example. But it's still a real behaviour change that I think we can avoid.

What I would like to see when adding a new x-death entry:

• Look for a matching one, matching on both queue and reason
• If there, increment its count and bring to the front of the list
• If not, create one with count=1 and add to the front of the list

...which I think is what @michaelklishin has done but with the addition of a counter and matching on reason too.

Matching on {queue, reason} and having a counter makes sense. Thanks.

Ready for another review:

• x-death events are now identified by {queue, reason}
• events now have a counter
• The docs are updated accordingly
• Channel#queueBind/2 was taken out

The logic of this looks OK to me now. Admittedly I haven't actually run it.

I have a bunch of style complaints, take them as seriously as you wish:

AutorecoveringChannel has grown an import for no obvious reason.

I would inline x_deaths_from_header/1 and x_death_not_for_queue/2, I don't think they make it any clearer. I also wonder if x_death_header/1 could become header(HeaderName, Headers, Default) and be moved to rabbit_basic, since I suspect it would be more generally useful.

Personally I don't find abstracting away the names of headers to be any clearer either (since you always have to mentally look it up) so I'd kill ?X_DEATH_HEADER and friends too. I know we do that elsewhere, but I don't like it elsewhere either 😄

I would rather the new field be called count rather than counter: it reads more naturally to me.

I gave the branch a try and did:

1. From the management UI, I created:

   • a my-dlx fanout exchange
   • a my-dlq queue bound to my-dlx
   • a test queue with a queue TTL of 1" and my-dlx as its dead-letter exchange

2. I used PerfTest to publish one message to test

3. The message ended in my-dlq after the 1" TTL. The x-death looked like:

   counter:        1
   reason:         expired
   queue:          test
   time:           1427292711
   exchange:       direct
   routing-keys:   e3e47d2e-8834-481f-8969-ec4125c1847c
   

4. Still using the management UI, I moved the message from my-dlq to test. Again, the message expired and ended up in my-dlq. The x-death counter has been incremented:

   counter:        2
   reason:         expired
   queue:          test
   time:           1427292711
   exchange:       direct
   routing-keys:   e3e47d2e-8834-481f-8969-ec4125c1847c
   

I tried again but this time with my-dlq configured like test to create a cycle. It was properly detected and the message was dropped.

I've renamed the field, removed the unused import and did some refactoring (introducing rabbit_basic:header/{2,3} and inlining).

The associated PR (#79) was merged to stable.