arobson / rabbot

Deprecated: Please see https://github.com/Foo-Foo-MQ/foo-foo-mq
MIT License
277 stars 129 forks source link

Multiple messages are NACK'ed #80

Closed ghost closed 6 years ago

ghost commented 7 years ago

My issue was:

  1. Push 2 messages to the queue (message 1 and message 2)
  2. NACK message 2, message 1 is still processing
  3. Message 1 and message 2 are redelivered. I Suppose this is wrong as message 1 was automatically NACK'ed.
  4. Message 1 ends processing and ACKS;
  5. Redelivered message 1 ACKS and queue fails with error Error: Channel closed by server: 406 (PRECONDITION-FAILED) with message "PRECONDITION_FAILED - unknown delivery tag 1

I'm getting error because message 1 was ACKED twice (due to unexpected redelivery).

I investigated the problem and this is because of this line in the /src/amqp/queue.js.

channel.nack( { fields: { deliveryTag: raw.fields.deliveryTag } }, true );

The 'true' at the end means allUpTo which is false by default but it is set as 'true' in the code. If we compare it to ack and reject:

channel.ack( { fields: { deliveryTag: raw.fields.deliveryTag } }, false );
channel.reject( { fields: { deliveryTag: raw.fields.deliveryTag } }, false ); 

Is there any reason to implement only nack this way?

astanciu commented 6 years ago

Any updates on this? I think I'm running into the same issue

astanciu commented 6 years ago

Here's code to replicate. It nacks both messages, when I would expect it to only nack msg 2:

require('dotenv').config()
const rabbit = require('rabbot');

rabbit.addConnection({
  user: process.env.RABBIT_USER,
  pass: process.env.RABBIT_PASS,
  host: process.env.RABBIT_HOST,
  port: process.env.RABBIT_PORT,
  timeout: 2000,
  vhost: process.env.RABBIT_VHOST,
  heartbeat: 10
});

const queueName = 'cert-q'
const exchange = 'phenix-x'
const routingKey = 'cert'

rabbit.addExchange(exchange, 'topic', {
  autoDelete: false,
  durable: true,
  persistent: true
})

rabbit.addQueue(queueName, {
  autoDelete: false,
  subscribe: false,
  durable: true,
  persistent: true,
  limit: 4,
  noBatch: true
})

rabbit.bindQueue(exchange, queueName, [routingKey])

rabbit.handle('test-event', handler, queueName)
rabbit.startSubscription(queueName, false, 'default')

sendMessage(1)
sendMessage(2)

function handler(msg) {
  id = msg.body.id
  console.log('got ' + id)

  if (id === 1) {
    // take a little longer to process
    setTimeout(() => {
      msg.ack()
    }, 1000)
  } else if (id === 2) {
    // nack right away
    msg.nack()
  }
}

function sendMessage(id) {
  const msg = {
    routingKey: routingKey,
    type: 'test-event',
    body: { id: id }
  }
  rabbit.publish(exchange, msg)
}
arobson commented 6 years ago

@ghost @astanciu - I am not able to repro the issue as described here using v2. I tested it with noBatch set to true and false and used the debug logger DEBUG=rabbot.* node test.js to watch all the operations. Message 1 was never redelivered, while 2 was.

For what it's worth, I did change the sample code to limit the number of times it would nack the 2nd message just to keep the redelivery from blowing up the console 😄