SpinGo / op-rabbit

The Opinionated RabbitMQ Library for Scala and Akka
Other
232 stars 73 forks source link

Is asynchronous nack thread blocking? #183

Open livius-ungureanu opened 4 years ago

livius-ungureanu commented 4 years ago

It looks like explicit nack can be used only synchronously and the only way to use it asynchronously is by supplying it as an implicit default RecoveryStrategy for BoundConsumerDefinition(i.e. the subscription).

So to consume some item asynchronously we should roughly use something like ack(Future{ body_process_result}) . The doc says that if the future fails then the RecoveryStrategy is applied(i.e the wanted nack is called). But this leads to https://github.com/SpinGo/op-rabbit/blob/ff9f8a25ef8fe94b348a7db9e88b5cd079b65035/core/src/main/scala/com/spingo/op_rabbit/impl/AsyncAckingRabbitConsumer.scala#L179 being hit which looks like thread blocking.

Are there any plans to improve this?

And a 2nd question: Is there any other approach to use nack asynchronously besides the approach above(also used in /op-rabbit/core/src/test/scala/com/spingo/op_rabbit/consumerSpec.scala test) ?

kw217 commented 4 years ago

I don't think this is actually a problem - the real work is handled by

https://github.com/SpinGo/op-rabbit/blob/9a681c4361f5b66385244bf1aa0079c2096c5969/core/src/main/scala/com/spingo/op_rabbit/impl/AsyncAckingRabbitConsumer.scala#L79-L82

which runs the work (subscription.handler) asynchronously as expected. Only if the work throws does anything interesting happen - in this case the exception becomes a ReceiveResult.Fail message, which invokes the recovery strategy synchronously in

https://github.com/SpinGo/op-rabbit/blob/9a681c4361f5b66385244bf1aa0079c2096c5969/core/src/main/scala/com/spingo/op_rabbit/impl/AsyncAckingRabbitConsumer.scala#L178-L185

The recovery strategy in our application (presumably typical) is just RecoveryStrategy.nack(requeue=true), which will complete immediately (ultimately it reduces to p.success(ReceiveResult.nack)). So the thread will only be blocked momentarily, which is not a problem.

I think this issue can probably be closed.

kw217 commented 4 years ago

To answer point 2, isn't it the case that if the body returns ReceiveResult.nack then op-rabbit will nack the message?

livius-ungureanu commented 4 years ago

thank you @kw217

Right.

So the main concern does not exist. Though it would have been preferred an implementation without thread blocking at all.

Regarding the 2nd point it would be nice if op-rabbit's RecoveryStrategy would be more flexible and allow the user to specify a reject with requeue = true/false as needed. Currently the user is forced to specify only one of them.

livius-ungureanu commented 4 years ago

We'd like to also have op-rabbit's thoughts on this.