noodlefrenzy / node-amqp10

amqp10 is a promise-based, AMQP 1.0 compliant node.js client
MIT License
134 stars 56 forks source link

connection error doesn't trigger error handler. Promise not resolved or rejected #342

Open swilliams-a3digital opened 6 years ago

swilliams-a3digital commented 6 years ago

If I have a bad url, or bad username / password combo, when I hit connect, I'd expect the .error() handler to get called. However, client.connect() just hangs forever. In the code snippet below, I'd expect a connection error to get logged to console by the .error(err= console.log(err)); If I turn on the debug environment variable I see your library generating experiencing errors back from the server, but they are not resolving or rejecting the .connect promise.

Code Snippet:

const AMQPClient = require("amqp10").Client; const Promise = require("bluebird");

const activeMQPolicy = require("amqp10").Policy; const client = new AMQPClient(activeMQPolicy.ActiveMQ);

const setUp = () => { console.log('doing setup'); return Promise.all([ client.createReceiver("USERS.destQueue"), client.createSender("USERS.sendQueue") ]).catch(err => { console.log('err'); }); }; console.log('connecting'); client.connect("amqp://user:password@localhost") .then(setUp).error(err => console.log(err));

DEBUG: LOGS:

amqp10:framing => @sasl-init(65) [mechanism='PLAIN' initialResponse=<Buffer 00 75 73 65 72 00 70 61 73 73 77 6f 72 64> hostname=null] +0ms amqp10:trace raw: [0000002602010000005341c01903a305504c41494ea00e00757365720070617373776f726440] +0ms amqp10:trace Rx: 0000001002010000005344c003015001 +1ms amqp10:framing <= @sasl-outcome(68) [code=1 additionalData=null] +0ms amqp10:trace raw: [005344c003015001] +0ms amqp10:connection Error from socket: AmqpAuthenticationError: SASL Failed: 1: undefined +0ms amqp10:client connection error: { AmqpAuthenticationError: SASL Failed: 1: undefined at Sasl._processFrame (/Users/swilliams/projects/sandbox/active-mq/amqp-client/node_modules/amqp10/lib/sasl.js:102:21) at Connection._processFrameEH (/Users/swilliams/projects/sandbox/active-mq/amqp-client/node_modules/amqp10/lib/sasl.js:43:49) at emitOne (events.js:115:13) at Connection.emit (events.js:210:7) at Connection._receiveAny (/Users/swilliams/projects/sandbox/active-mq/amqp- name: 'AmqpAuthenticationError', message: 'SASL Failed: 1: undefined' } +1ms amqp10:client disconnected +0ms amqp10:connection stateChange: IN_SASL => DISCONNECTED , reason: error +0ms

princjef commented 6 years ago

Have you tried replacing your .error() on the last line with .catch()? Calling .error() on a Bluebird promise only catches operational errors. .catch() is also the standard way of catching errors in ES6 Promises, so you can write code without worrying what kind of Promise it is.

amarzavery commented 6 years ago

I noticed one more thing. Even though the library is using Bluebrid internally, it is not neccessary for you to use it. You can use standard promise that is available in node.js. Instead of using the .spread and .error methods you can chain things with .then() and have a .catch() in the end.

swilliams-a3digital commented 6 years ago

I wasn't able to get error or catch handlers to catch trigger on connection problems. I ultimately worked around the issue using .on to handle various events. e.g.: const AMQPClient = require('amqp10').Client; const activeMQPolicy = require('amqp10').Policy; this.client = new AMQPClient(activeMQPolicy.ActiveMQ); this.client.on('connected', () => { // handle first time connect and reconnect } this.client.on(AMQPClient.ErrorReceived, (err) => { console.log(err received ${err}); // handle error }); this.client.on(AMQPClient.ConnectionClosed, () => { // handle disconnect }

tony-gutierrez commented 6 years ago

Yeah, in initial tests with turning off the connection, it takes a long time to get any event. I think it is dependent on the remoteTimeout, which we cannot change. As I wanted to be more proactive on network drops, I just turned off all reconnect, and do a disconnect/connect on a timeout set after the last message received.

It would be nice to be able to hook into heartbeat errors, or something that would warn you earlier of issues.