Problem with channel ack

[hooblei]

Sorry me agian - this time with a documentation and logic problem within a channel/consumer ack. The docs state:

\amqphp\CONSUMER_ACK - this const is a signal to the Channel to respond with basic.ack (multiple=false)

but this is not the case: https://github.com/BraveSirRobin/amqphp/blob/master/src/amqphp/Channel.php#L406 - so looks like all acks will be send with multiple=true, which makes it impossible to only ack particular messages and leave messages 'unacknowledged' (.. for whatever)

btw - I think It would be a good idea to note something about the channel ackBuffer in the consumer docs - it took me some time to figure out why the channel was not sending any acks while I was playing with a single message g

[BraveSirRobin]

Hi Hooblei,

No worries, I'm glad that people are using Amqphp!!

The ackBuffer stuff was added quite recently, you're right that I should update the docs about this, I'll try to get some time over the weekend.

Regarding the multiple=true issue, I'm not totally convinced this is a "bug", the way the library works is that if the ackBuffer is set to 1, acks are sent for each message, so the fact that mutliple=true doesn't really matter, as there's only one outstanding message to ack.

I see your point though, with the Channel set up as it is now, you aren't able to deal with sending acks yourself, so if you want to do something non-standard (say, acking out of order) then you're out of luck.

I think maybe what we need to do is add another return value, say \amqphp\CONSUMER_IGNORE, this way the Channel would know that it should not ack that message, leaving the user to be able to ack it whenever they like. On receiving a CONSUMER_IGNORE, the Channel would immediately flush existing acks, and store a local flag so that the next ack it receives should be sent multiple=false. Does this sound about right?

I've got a few bits and pieces to look at over the weekend, so I'll look at sorting this issue out then. If you've got any thoughts or suggestions on how best to sort this issue, please post them back to this bug report.

Thanks,
--Robin

[hooblei]

Hi Robin,

yes - with a ackBuffer set to 1 or 0, the channel will send a ack for each CONSUMER_ACK, that will return from handleDelivery

msg 1 - return CONSUMER_ACK -> channel ack
msg 2 - no return (implicit drop, reject or re-queue, what?)
msg 3 - return CONSUMER_ACK -> channel ack - this will also ack msg 2 since multiple=true

I'm also not sure if this is a bug, this issue only matters if one decides to consume with no-ack=false and I think in this case the consumer should return a explicit ACK/DROP/REJECT on each message. I just run in this yesterday, when I was porting/evaluating some existing message handlers for amqp that where created for a different queue system.

I think the CONSUMER_IGNORE isn't really needed since a null/no return would implicit or equal a CONSUMER_IGNORE, but would not be bad if it that would be defined (const CONSUMER_IGNORE = 0; + $response = (int) $cons->handleDelivery(... ) and yes - the described transition sounds right.

Markus

[BraveSirRobin]

Hi Markus,

Thinking about this more yesterday, I think you're right, CONSUMER_IGNORE isn't needed, we just need to make sure we only ack (or reject) messages that we're specifically told to. I'll look in to this at the weekend, should be a fairly simple fix.

Thanks,
--Robin

[BraveSirRobin]

Hi Markus,

I've made some changes to the Channel class which I hope should solve this issue, they're in this commit

In the end, I have not implemented this the way we discussed, as this did not work. The implementation has ended up being a bit simpler, the only change is to send multiple=true only in cases where $ackBuffer > 1. I tried out the implementation discussed above, whereby the Channel would send single acks with multiple=false in order to create "ack gaps", however this didn't work well, because the "gaps" would be closed up by the Channel sending subsequent multiple=true (n)acks.

As things stand now, I suggest that you set the $ackBuffer to 1, that way all (n)acks will be sent multiple=false, and any ack gaps that you wish to create won't get implicitly filled in by multiple=true.

Please would you download the HEAD version and run some tests to see if this sorts your issue, if there are still problem let me know and I'll take another look.

Thanks,
--Robin

[hooblei]

:)) - mutliple=($this->ackBuffer > 1) - was the first thing that came to my mind when I was looking the first time in flushAcks, but as you mentioned, this only works for a ackBuffer <= 1 and I'm fully comfortable with this. I also tested the message handler that brings up this issue and it worked.

Thanks,
Markus

[BraveSirRobin]

Lovely, thanks Markus :-)

I've written up and explanation of the ackBuffer feature (at the bottom of the page), let me know if you think anything else needs to be added.

Thanks,
--Robin

[hooblei]

Ah - good explanation - thanks

Markus