Fix channel overflow

[unframework]

https://github.com/postwait/node-amqp/issues/205

The client library is ignoring the max channel ID limit sent by the server (65535) instead of properly wrapping the ID back around to 1. When the client-generated channel ID hits the limit, the server kicks out the client with an error.

Changed to wrap-around the channel ID properly, while avoiding already-used IDs.

[amackera]

I was still able to reproduce this issue after applying this PR. It seems as though the channels themselves are never closed. I'll try to write a test case to reproduce what I'm talking about.

[unframework]

Yes, that's an unrelated issue we had to work around. Just do .close() after .destroy() on queues and exchanges - did not see it in the documentation, but queue and exchange objects are subclasses of channels. Maybe .destroy() is supposed to do it automatically but we didn't dig deeper. But again - that issue is unrelated.

[amackera]

Roger that, good tip about .close().

On Tue, Sep 3, 2013 at 4:09 PM, unframework notifications@github.com wrote:

Yes, that's an unrelated issue we had to work around. Just do .close() after .destroy() on queues and exchanges - did not see it in the documentation, but queue and exchange objects are subclasses of channels. Maybe .destroy() is supposed to do it automatically but we didn't dig deeper. But again - that issue is unrelated.

Reply to this email directly or view it on GitHub: https://github.com/postwait/node-amqp/pull/237#issuecomment-23742839

[aahoughton]

Ha, I was about to submit a pull request for the same issue. This implementation is prettier. :)

[andris9]

Any progress on this issue? I can confirm (using node-amqp v0.1.8) that once connection.channelCounter reaches over 2^16 CHANNEL_ERROR is thrown.

To test this, I set the channelCounter value to 65536 - 3 and opened some exchanges and queues.

Error: CHANNEL_ERROR - unexpected method in connection state running
    at Connection._onMethod (/var/app/node_modules/amqp/lib/connection.js:468:15)
    at AMQPParser.self.parser.onMethod (/var/app/node_modules/amqp/lib/connection.js:122:12)
    at AMQPParser._parseMethodFrame (/var/app/node_modules/amqp/lib/parser.js:361:10)
    at frameEnd (/var/app/node_modules/amqp/lib/parser.js:92:16)
    at frame (/var/app/node_modules/amqp/lib/parser.js:77:14)
    at AMQPParser.header [as parse] (/var/app/node_modules/amqp/lib/parser.js:63:14)
    at AMQPParser.execute (/var/app/node_modules/amqp/lib/parser.js:136:21)
    at Connection.<anonymous> (/var/app/node_modules/amqp/lib/connection.js:160:21)
    at Connection.EventEmitter.emit (events.js:95:17)
    at Socket.EventEmitter.emit (events.js:95:17))

[andris9]

My take on fixing this: https://github.com/andris9/node-amqp/commit/84435b7a50632a07cac3471466ccf22ea82e9e97

[unframework]

@andris9 - just want to point out that beside spacing/comment/JSLint differences, my commit also adds a check in the handshake to fail-fast if the server does try to specify alternate channelMax. Also, the code throws an exception instead of reporting an error event when channels are depleted, specifically to signify that it is a serious developer error (not the same as e.g. simple network disconnects/etc). The latter can be debated, though, heh.

[andris9]

I agree and this is why I didn't submit a separate pull request. I couldn't use your patch directly as it is somewhat outdated, the code is moved from amqp.js to connection.js. Adding just a small function to reuse channel id values seemed simpler than fixing the patch conflicts.

[aheckmann]

Please add a test to this PR. The odds of any code being accepted without tests are slim-to-none.

[postwait]

took 284