Is unconditional acknowledgement in tutorial two the right thing to do?

[dvdsk]

On the tutorials page (java) at https://www.rabbitmq.com/tutorials/tutorial-two-java.html there is an example of how to use Acknowledging. In this example the ack is always send. Shouldn't it be send only if the work succeeds? (I am a beginner at java however I think the finally branch always runs)

channel.basicQos(1); // accept only one unack-ed message at a time (see below)

DeliverCallback deliverCallback = (consumerTag, delivery) -> {
  String message = new String(delivery.getBody(), "UTF-8");

  System.out.println(" [x] Received '" + message + "'");
  try {
    doWork(message);
  } finally {
    System.out.println(" [x] Done");
    channel.basicAck(delivery.getEnvelope().getDeliveryTag(), false);
  }
};
boolean autoAck = false;
channel.basicConsume(TASK_QUEUE_NAME, autoAck, deliverCallback, consumerTag -> { });

I would solve this by taking the code out of the finally block and adding a catch block. Then escape the entire function on an exception in doWork(message).

Apologies if this should have been posted in another repo, I could not find from which repo the above code example originated.

[lukebakken]

here is the code to which you refer. I think it's reasonable to change it here and here. We'd be happy to review pull requests in both repositories.

What do you think @acogoluegnes ?

[michaelklishin]

This is not a mistake. This approach can be valid as well: not every delivery should be requeued. Of course, this begs the question "why don't you just use automatic acknowledgements then" and it's fair.

Perhaps the approach in #255 makes more sense in a tutorial but the current implementation has its merits as well.

[michaelklishin]

The code in #255 does not result in delivery requeueing, it just leaves deliveries in limbo upon exception. That is worse than what we currently have.

[dvdsk]

What if its turned into a try catch, sending a nack on any exception then returning early from the callback. Instead of in a finally clause we would acknowledge after the try catch:

try {
  doWork()
}
catch (Exception e){
  nack()
  return
}
// only pass here if the work was done
ack()

[michaelklishin]

That would make more sense but note that to RabbitMQ, a positive acknowledgement is no different from a negative one if requeue is set to false. So you'd simply use a try/catch instead of a try/finally (which I would consider an improvement).