Errors silently swallowed in 0.2

[glenjamin]

Hello, I ran into this while teaching an introductory workshop - it was extremely confusing for the attendees!

Consider the following example script:

var amqp = require('amqp');
var url = process.env.AMQP_URL || 'amqp://guest:guest@localhost:5672/%2F';

var reconnect = !!process.env.AMQP_RECONNECT;

console.warn("Connecting to %s", url);
var connection = amqp.createConnection({ url: url }, { reconnect: reconnect });

connection.on('ready', function() {
    console.warn("Connected to server");

    connection.exchange('test', {type: 'topic'}, function(x) {
        console.warn("Exchange declared, publishing...");

        x.pubilsh('note', 'spelt the method wrong');
    });
});

Notice the code has an error - the publish method is spelt incorrectly.

If you set the AMQP_URL environment variable and run this, you'll see it start running, then die without errors and an exit code of zero.

Now set AMQP_RECONNECT=1 and run again, you'll see it keep on running and failing without mentioning any of the failures.

From what I can tell, this is related to the connection error handling code, which does not distinguish between connection failures, and actual "you did this wrong" failures - the usual behaviour I'd expect from an EventEmitter is that since I've not attempted to explicitly handle the error event, any errors will cause my program to die very loudly.

Adding the following restores the default EventEmitter behaviour, but obviously this clashes with the reconnect code.

connection.on('error', function(err) { throw err; })

[postwait]

Can you try this:

diff --git a/lib/connection.js b/lib/connection.js
index 24aed64..023ca3c 100644
--- a/lib/connection.js
+++ b/lib/connection.js
@@ -248,6 +248,8 @@ Connection.prototype.addAllListeners = function() {
         }, backoffTime);
       } else {
         self.removeListener('error', backoff);
+        var listeners = self.listeners('error');
+        if(listeners === null || listeners.length === 0) throw(e);
       }
     }
   });

[glenjamin]

Cheers for the quick response.

That solves the reconnect: false case, although the error source ends up looking a touch odd:

> node silent.js
Connecting to amqp://guest:guest@192.168.73.133/conglomerate
Connected to server
Exchange declared, publishing...

/Users/gma08/Development/GitHub/conglomerate-node-client/node_modules/amqp/lib/connection.js:252
        if(listeners === null || listeners.length === 0) throw(e);
                                                               ^
TypeError: Object #<Exchange> has no method 'pubilsh'
    at Exchange._openCallback (/Users/gma08/Development/GitHub/conglomerate-node-client/silent.js:15:11)
    at Exchange._onMethod (/Users/gma08/Development/GitHub/conglomerate-node-client/node_modules/amqp/lib/exchange.js:99:16)
    at Exchange.Channel._onChannelMethod (/Users/gma08/Development/GitHub/conglomerate-node-client/node_modules/amqp/lib/channel.js:85:12)
    at Connection._onMethod (/Users/gma08/Development/GitHub/conglomerate-node-client/node_modules/amqp/lib/connection.js:424:28)
    at AMQPParser.self.parser.onMethod (/Users/gma08/Development/GitHub/conglomerate-node-client/node_modules/amqp/lib/connection.js:133:12)
    at AMQPParser._parseMethodFrame (/Users/gma08/Development/GitHub/conglomerate-node-client/node_modules/amqp/lib/parser.js:360:10)
    at frameEnd (/Users/gma08/Development/GitHub/conglomerate-node-client/node_modules/amqp/lib/parser.js:93:16)
    at frame (/Users/gma08/Development/GitHub/conglomerate-node-client/node_modules/amqp/lib/parser.js:78:14)
    at AMQPParser.header [as parse] (/Users/gma08/Development/GitHub/conglomerate-node-client/node_modules/amqp/lib/parser.js:64:14)
    at AMQPParser.execute (/Users/gma08/Development/GitHub/conglomerate-node-client/node_modules/amqp/lib/parser.js:137:21)

However there's no change to the reconnection behaviour.

> AMQP_RECONNECT=1 node silent.js
Connecting to amqp://guest:guest@192.168.73.133/conglomerate
Connected to server
Exchange declared, publishing...
Connected to server
Exchange declared, publishing...
Connected to server
Exchange declared, publishing...
Connected to server
Exchange declared, publishing...
Connected to server
Exchange declared, publishing...
Connected to server

With the fix about, it still feels like there's a couple of issues here:

• non-amqp-related errors being bundled into the connection object due to try..catch
• auto-reconnect triggering on application errors as well as connection errors

For example, if I try and declare an exchange with a different type to the server's I get the following error:

Error: PRECONDITION_FAILED - cannot redeclare exchange 'test' in vhost 'conglomerate' with different type, durable, internal or autodelete value

Which is of course, correct.

However, if i enable reconnection, the app continually retries the connection, which always results in the same failure.

I'm not really sure what the "correct" behaviour should be!

[gabrielf]

It would be nice if the README mentioned how one is supposed to do error handling. I also discovered the error event by reading the module's source code for someone used to callbacks that get an error as their first parameter it is not immediately obvious how I should detect if a certain rabbit call fails.