Connection does not close cleanly

[cmoesel]

Even when calling the connection.end(), the connection to the RabbitMQ server does not close cleanly, causing a warning in the logs:

=WARNING REPORT==== 27-Sep-2013::17:17:44 ===
closing AMQP connection <0.25554.0> (10.0.2.2:60134 -> 10.0.2.15:5672):
connection_closed_abruptly

This is easily reproduced with the following code:

var amqp = require('amqp');

var connection = amqp.createConnection({ host: 'localhost' });
connection.on('ready', function () {
  connection.end();
  connection.on("close", function() {
    console.log("done.");
  });
});

[cmoesel]

I found the issue. According to the AMQP spec, the client should send the connection close method to the server whenever closing a connection. The amqp-node library just closes the socket (without sending any notice to the server).

I have a patch ready, but there is one thing I would like some feedback about. According to AMQP spec, the server responds with a close-ok. This "tells the recipient that it is safe to release resources for the connection and close the socket." So... should I wait for the "close-ok" before closing the socket? Should I put in a fallback in case we never receive the "close-ok" (maybe a timeout of some sort)?

[crzidea]

Does this issue fixed? I tried the following code:

connection.end();

But the connection reconnected automatically. Then I tried this:

connection.implOptions.reconnect = false;
connection.end();

But I got an error:

Error: read ECONNRESET
    at errnoException (net.js:901:11)
    at TCP.onread (net.js:556:19)

@postwait Would you handle this PR?

[cmoesel]

@crzidea: My patch does not address the bug you just described. In my case the connection did end-- it just didn't end cleanly. I don't think my patch will fix the issue you describe-- that sounds like a different issue.

[crzidea]

@cmoesel Thank you for your reminding. I will check it..

[theartoflogic]

Is this fix going to be merged any time soon? I don't see it in master or the latest release (0.1.8).

[cmoesel]

I just discovered that a few tests were failing with this fix applied. This might be why it has not been merged yet. I've just update the pull request with some minor fixes and now all tests pass. Hopefully this improves the chances of being merged!

[cmoesel]

Closing since #248 was merged a while ago.