search

ruby-amqp / bunny

Bunny is a popular, easy to use, mature Ruby client for RabbitMQ
Other
1.39k stars 303 forks source link

Redeliver publisher confirms acks again and again from 0 till current tag #615

Closed kinnalru closed 3 years ago

kinnalru commented 3 years ago

I wrote some benchmarks and met some strage behaviour: In confirm_select mode sometimes callback called with already acked delivery tag. I start to investigate and find this code: https://github.com/ruby-amqp/bunny/blob/master/lib/bunny/channel.rb#L1792

When rabbitmq sends me an confirm with multiple equals to true confirmed_range_start setted to @delivery_tag_offset + 1. But @delivery_tag_offset is always 0 without network recoveries!

So I have such results: Send 16000 very small messages ConfirmSelect callback called 119567 times - each delivery_tag was acked ~ 9 times.

Which workaround can I use? The main fix I see to pass tag|multiple|nack triplet to user in callback without changes, avoiding useless work to iterate through tags from beginning over and over

michaelklishin commented 3 years ago

I don't see a solution that would not involve keeping track of the "most recently seen" delivery tag which would be reset upon connection recovery. Then we'd use that as the lower range boundary. How does that sound?

I'm not sure if most users would appreciate being exposed to the multiple property value. That would also be a breaking public API change. It should be very rare to see a confirmation of many (say, dozens or hundreds) of messages at a time, so invoking the callback once for every delivery still seems like a reasonable design choice to me.

kinnalru commented 3 years ago

Hi @michaelklishin With low requests frequency RabbitMQ don't use multiple attribute on acks. But sometimes our service serves about 500 rsp. and RabbitMQ sometimes sends multiple attribute. So after a while without network failures we receive dozens of confim_select callback for delivery_tag most of which was succesefuly acked some hours ago ex: from 0 to 16k acks. :(

Are you intresting in PR with little complex logic in handle_ack_or_nack ? I testing draft impl now.

michaelklishin commented 3 years ago

I know when RabbitMQ would confirm N messages at once: when there are several of them confirmed by target queues since the last time a delivery tag was sent out. So indeed it would take a certain ingress message rate.

I would be interested in a PR that makes handling of basic.acks with multiple set to true correctly.

kinnalru commented 3 years ago

I would be interested in a PR that makes handling of basic.acks with multiple set to true correctly.

Morning.
We create PR for this issue.

@michaelklishin please give us some comments