twingly / twingly-amqp

:bus: Ruby gem for RabbitMQ subscribing and publishing
0 stars 0 forks source link

`cancel!` does nothing for a "non-blocking" subscriber #86

Closed walro closed 11 months ago

walro commented 3 years ago

Noticed this while using twingly-amqp in the test of another project.

The problem is that cancel! only sets @cancel = true, as seen in: https://github.com/twingly/twingly-amqp/blob/7b8fa545f336a072bd5174b6bd4c86eafee81f1b/lib/twingly/amqp/subscription.rb#L69

The only time we cancel a consumer is at: https://github.com/twingly/twingly-amqp/blob/7b8fa545f336a072bd5174b6bd4c86eafee81f1b/lib/twingly/amqp/subscription.rb#L41-L45

As seen above, we only consumer.cancel in the blocking case. In the non-blocking case there will but a consumer running "forever".

roback commented 3 years ago

An ugly solution 😄

if blocking 
  sleep 0.01 until cancel? 

  consumer.cancel
else
  Thread.new do
    sleep 0.01 until cancel? 

    consumer.cancel
  end
end 
Chrizpy commented 3 years ago

Is this still an issue?

walro commented 3 years ago

Yep!

roback commented 11 months ago

When we added som specs for the #cancel! method in https://github.com/twingly/twingly-amqp/pull/89, we skipped the specs for non-blocking subscribers, as #cancel! doesn't work then. However, we do have some other tests which depends on the non-working #cancel! method: https://github.com/twingly/twingly-amqp/blob/9e7312a5f74d77964def689f7716fb9c1cccc4c3/spec/integration/twingly/amqp/subscription_spec.rb#L274-L305

I guess we're just lucky that these tests work right now. The above tests caused some other Subscriber-related tests to fail in unexpected ways when I tried to switch over to quorum queues in #100

roback commented 11 months ago

We use publisher confirms in our specs to wait for messages to be handled before we continue with the tests. However, I think that we have assumed that the publisher waits for the messages to be confirmed by a consumer (like we do in the specs above), but that's not the case according to the documentation:

When Will Published Messages Be Confirmed by the Broker? [...] For routable messages, the basic.ack is sent when a message has been accepted by all the queues. For persistent messages routed to durable queues, this means persisting to disk. For quorum queues, this means that a quorum replicas have accepted and confirmed the message to the elected leader.

The publisher just waits for the RabbitMQ server to persist the message the relevant queues, not for the consumers to acknowledge it.

This might be why I'm seeing some test failures when experimenting with quorum queues, and also why we've seen a few intermittent test failures in other projects, for example https://github.com/twingly/dawson/issues/225 (although that doesn't happen that often).

Edit: I just realised that this should be its own issue, so I'll create a new one about this (https://github.com/twingly/twingly-amqp/issues/102) :)