postwait / node-amqp

[UNMAINTAINED] node-amqp is an AMQP client for nodejs
MIT License
1.69k stars 357 forks source link

queueBindOk event called twice #269

Closed waynebrantley closed 10 years ago

waynebrantley commented 10 years ago

You will see 6 messages arrive in the queue. 'declare' is output once. 'bind' is output twice

require('../setup').Init('Hello World.');
var connect = require('amqp').createConnection();
var messages = [];
messages.push(' (\\-/)');
messages.push(" ('.')  –> hello world");
messages.push('(") (")');

connect.on('ready', function() {
    var ex = connect.exchange();
    var q = connect.queue('hello');
    q.on('queueDeclareOk', function(args) {
        console.log('declare');
        q.bind('#');
        q.on('queueBindOk', function() {
        console.log('bind');
            messages.forEach(function(message) {
                ex.publish('hello', message, {});
            });
        });
    });
});
davedoesdev commented 10 years ago

The second call is a result of this in Queue.prototype._onMethod:

    case methods.queueDeclareOk:
      this.state = 'open';
      this.name = args.queue;
      this.connection.queues[this.name] = this;

      // Rebind to previously bound exchanges, if present.
      // Important this is called *before* openCallback, otherwise bindings will happen twice.
      // Run test-purge to make sure you got this right
      _.each(this._bindings, function(exchange, exchangeName){
        _.each(exchange, function(count, routingKey){
          self.bind(exchangeName, routingKey);
        });
      });

I'm guessing your bind gets in first.

davedoesdev commented 10 years ago

This only displays bind once:

var connect = require('amqp').createConnection();
var messages = [];
messages.push(' (\\-/)');
messages.push(" ('.')  -> hello world");
messages.push('(") (")');

connect.on('ready', function() {
    var ex = connect.exchange();
    var q = connect.queue('hello', function () {
        console.log('declare');
        q.bind('#');
        q.on('queueBindOk', function() {
            console.log('bind');
            messages.forEach(function(message) {
                ex.publish('hello', message, {});
            });
        });
    });
});
waynebrantley commented 10 years ago

Seems like original should not display twice though, huh?

davedoesdev commented 10 years ago

Yes, it doesn't seem right to display twice. However, this module is quite a high-level wrapper over amqp so it may be that queueBindOk is caught in order to rebind the queue when reconnecting (or something like that). It seems each queue uses a separate channel so waiting until the channel is open (which is what the callback to queue does) looks like the 'approved' way of doing it.