Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't reconfirm already acked messages when multiple is true #617

Merged
merged 7 commits into from Jun 16, 2021
Merged

Don't reconfirm already acked messages when multiple is true #617

merged 7 commits into from Jun 16, 2021

Conversation

kinnalru
Copy link
Contributor

In current implementation library acks all tags starting from tag 1 (plus channel recovery offset) every time it gets multiple flag regardless of previously acknowledged tags. So when multiple flag occured ex. at delivery_tag 1000 (after 990 aked), we receive confirm_select callback 1000 times. And we receive this call callback again and again (hudren thousands times) while processing just 16k messages.

After some research of other language implementation (java and python) we made a little fix to resolve this issue. RabbitMQ protocol extension strgongly enforces sequenced delivery_tag generation on publishing. There is @unconfirmed_set filled with not-yet-acked tags (previously acked tags already removed from it) so we just need to ack tags from first (smallest) unconfirmed position till currentlry delivered with multiple flag to fit RabbitMQ publisher confirms protocol extension.

There is additional test case to test and compare old and new behaviour. It is not for enclusion in resulting PR - just for experiments. You can change messages count to higher value when difference increased drastically.

Fixes #615

@kinnalru kinnalru mentioned this pull request Jun 14, 2021
Closed
@michaelklishin
Copy link
Member

Thank you for the thorough PR. Note that you can add benchmarks under benchmarks, e.g. we can keep the one that tests the proposed implementation there. Or we can drop it as it's served its purpose.

My test runs on a 4-year old CPU suggest that once the number of messages published goes above 20K or so, the new implementation begins significantly outperforming. In general, this implementation should be acceptable
for most users even if Set#min is not optimised to be a constant time operation.

So I'd be happy to merge once you remove the temporary tests (or move one of them to ./benchmarks). Thanks again for
putting in the time and effort 👏

@yurusov
Copy link
Contributor

yurusov commented Jun 16, 2021

I deleted those specs, they've been created only for this PR, to show difference in performance. Thanks for your time and quick response!
P.S. Sorry for extra pr I made by mistake.

@michaelklishin michaelklishin merged commit 13706d9 into ruby-amqp:master Jun 16, 2021
michaelklishin added a commit that referenced this pull request Jun 17, 2021
@michaelklishin
Add #617 to release notes
cad7442
michaelklishin added a commit that referenced this pull request Jun 17, 2021
@michaelklishin
#617 was contributed by multiple people, so credit where due
45303a8
romuloceccon added a commit to romuloceccon/bunny that referenced this pull request Jul 13, 2022
@romuloceccon
Avoid race between multi-message ack and publish
e7f8feb 
PR ruby-amqp#617 introduced an optimization for multi-message acks but failed
to protect @unconfirmed_set. In edge cases #basic_publish would
attempt to modify it during iteration with Enumerable#min:

    <ruby>/lib/ruby/3.1.0/set.rb:522:in `add': can't add a new key
    into hash during iteration (RuntimeError)
    from <bunny>/lib/bunny/channel.rb:555:in `block in basic_publish'
    from <bunny>/lib/bunny/channel.rb:554:in `synchronize'
    from <bunny>/lib/bunny/channel.rb:554:in `basic_publish'
    from <bunny>/lib/bunny/exchange.rb:141:in `publish'
romuloceccon added a commit to romuloceccon/bunny that referenced this pull request Jul 13, 2022
@romuloceccon
Avoid race between multi-message ack and publish
ccb99a4 
PR ruby-amqp#617 introduced an optimization for multi-message acks but failed
to protect @unconfirmed_set. In edge cases #basic_publish would
attempt to modify it during iteration with Enumerable#min:

    <ruby>/lib/ruby/3.1.0/set.rb:522:in `add': can't add a new key
    into hash during iteration (RuntimeError)
    from <bunny>/lib/bunny/channel.rb:555:in `block in basic_publish'
    from <bunny>/lib/bunny/channel.rb:554:in `synchronize'
    from <bunny>/lib/bunny/channel.rb:554:in `basic_publish'
    from <bunny>/lib/bunny/exchange.rb:141:in `publish'
romuloceccon added a commit to romuloceccon/bunny that referenced this pull request Jul 13, 2022
@romuloceccon
Avoid race between multi-message ack and publish
6fec044 
PR ruby-amqp#617 introduced an optimization for multi-message acks but failed
to protect @unconfirmed_set. In edge cases #basic_publish would
attempt to modify it during iteration with Enumerable#min:

    <ruby>/lib/ruby/3.1.0/set.rb:522:in `add': can't add a new key
    into hash during iteration (RuntimeError)
    from <bunny>/lib/bunny/channel.rb:555:in `block in basic_publish'
    from <bunny>/lib/bunny/channel.rb:554:in `synchronize'
    from <bunny>/lib/bunny/channel.rb:554:in `basic_publish'
    from <bunny>/lib/bunny/exchange.rb:141:in `publish'
@romuloceccon romuloceccon mentioned this pull request Jul 13, 2022
Merged
@michaelklishin michaelklishin mentioned this pull request Apr 4, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelklishin michaelklishin michaelklishin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Redeliver publisher confirms acks again and again from 0 till current tag
3 participants
@kinnalru @michaelklishin @yurusov