Layered API to expose channel operations #60

Open
squaremo opened this Issue Dec 30, 2011 · 10 comments

Comments

Projects
None yet
4 participants
Contributor

squaremo commented Dec 30, 2011

This is a problem resulting from caching queues and hiding channels: message prefetch is defined per channel, but a queue (= one channel) may have more than one consumer. If queue.subscribe and the option autoAck: false are used, it sets the prefetch to 1, meaning that another message won't be sent until the previous has been acknowledged.

If there's more than one consumer, then it's easy for one of them to unjustly starve the others by not acknowledging a message. Say, for example, each consumer is piping messages through a stream, and only acknowledging them once the message has successfully written; if the stream is paused, the acknowledgment won't be made, and no other consumer will receive a message either, because they are all sharing the channel and thus the prefetch.

One possible solution to this is to unhide channels, so to allow a channel per consumer. The API as is can be kept as a convenience, and channel exposed for when more fine-grained control is needed. Thoughts? (or would you prefer to wait for a pull request ..)

Owner

postwait commented Dec 30, 2011

Hiding channels is indeed problematic. If you see a way to expose them, but not increase the complexity for the current use cases, I'm all for it. The majority of node-amqp users (AFAICT) make very simplistic use of AMQP itself and I'd rather not confuse them (read: more support requests).

Theo Schlossnagle
http://lethargy.org/~jesus/

On Friday, December 30, 2011 at 12:04 PM, Michael Bridgen wrote:

This is a problem resulting from caching queues and hiding channels: message prefetch is defined per channel, but a queue (= one channel) may have more than one consumer. If queue.subscribe and the option autoAck: false are used, it sets the prefetch to 1, meaning that another message won't be sent until the previous has been acknowledged.

If there's more than one consumer, then it's easy for one of them to unjustly starve the others by not acknowledging a message. Say, for example, each consumer is piping messages through a stream, and only acknowledging them once the message has successfully written; if the stream is paused, the acknowledgment won't be made, and no other consumer will receive a message either, because they are all sharing the channel and thus the prefetch.

One possible solution to this is to unhide channels, so to allow a channel per consumer. The API as is can be kept as a convenience, and channel exposed for when more fine-grained control is needed. Thoughts? (or would you prefer to wait for a pull request ..)


Reply to this email directly or view it on GitHub:
#60

Contributor

squaremo commented Dec 30, 2011

The majority of node-amqp users (AFAICT) make very simplistic use of AMQP itself and I'd rather not confuse them (read:
more support requests).

Roger that. I'll work it through in a branch and see what I can come up with.

Contributor

squaremo commented Jan 4, 2012

Theo,

I've started a branch api_cleanup which has a channel interface as well as keeping the connection.queue, connection.exchange etc. methods.

The main changes (mostly in squaremo/node-amqp@6d9f27a) are:

  • queues and exchanges no longer inherit from Channel, but delegate to it;
  • Channel gets two methods for sending AMQP methods (one for when it should expect a reply, e.g., queue.bind, and one for when it shouldn't e.g., basic.publish), two special-purpose methods for registering and unregistering consumers, and one for publishing (which takes content as well as the method); everything else is then implemented using those
  • connection has a createChannel method, making Channel effectively public.

I have also got rid of the events named for AMQP methods, which I always found a bit weird (e.g., subscribe is done when the 'basicQosOk' event fires?). The same effect can be had using the promise returned from the channel (and thereby the queue and exchange) methods.

Here's an example of a test that I've rewritten:
https://github.com/squaremo/node-amqp/blob/api_cleanup/test/test-simple.js

As you can see not much has changed, really just the means of synchronising on AMQP commands (now using the promise return value, as described above).

Contributor

squaremo commented Jan 11, 2012

I think I will go ahead and rewrite the rest of the tests and the README. I'm also thinking adding some methods to Channel for the various AMQP operations (notably channel.flow, basic.qos, basic.reject, and Rabbit's extensions like confirmations).

Do shout if you think this is, er .. divergent ..

Contributor

squaremo commented Jan 18, 2012

Some tests (and presumably application code) rely on events such as 'queueDeclareOk'; this is distinct from a callback, which receives the "completed" queue rather than the queueDeclareOk sent by the server (which has a message count).

I can see three options:

  1. Consistency: queue() and exchange() return promises for the queueDeclareOk and exchangeDeclareOk objects sent back by the server, and take a callback which is given the 'completed' queue or exchange. This would preclude some race conditions that are currently easy to create, since the queue or exchange would not be available until it had been successfully declared.
  2. Least change (for present users): queue() and exchange() return 'incomplete' queues and exchanges as before, and e.g., queues emit 'queueDeclareOk' events as before.
  3. For the sake of completeness: queues and exchanges are returned as before, and are also promises, with addCallback() giving access to e.g., queueDeclareOk. Queues and exchanges don't emit these events.

Personally I favour consistency. One-off events should be represented in the API as callbacks (or promises) rather than events.

Contributor

ivolo commented Apr 30, 2013

@squaremo, I wanted to thank you for all your work in this direction. I was wondering if there could be more movement to finish this up, and merge in the last year's changes to node-amqp.

We have a thousands of queues (which AFAIK is a normal use case), and having a channel per queue results in hundreds of MB of extra overhead for the server. Here's a reference RabbitMQ thread about this: http://lists.rabbitmq.com/pipermail/rabbitmq-discuss/2013-April/026486.html . In addition, setting a prefetchCount that spans multiple subscriptions becomes impossible.

Thanks again!

Contributor

ivolo commented Apr 30, 2013

I was hoping that the global flag being set to true would allow me to get the prefetchCount across subscriptions, but it turns out thats global connection limits is one of the features that RabbitMQ does not implement from the AMQP standard.

So without a layered channel API, it's impossible to tell the server that you only want X messages at a time across multiple subscriptions.

Contributor

squaremo commented May 1, 2013

@ivolo Ops, I think I must have put this down somewhere and forgotten about it. It's somewhat behind the state of the art, now; on the other had, the changes were not drastic in the first place and could probably be recapitulated on master.

The difficulty is really that I don't think, present company excepted, it would be a welcome change. To do it properly would likely be a breaking change (I would speculate that many applications using node-amqp accidentally depend on the quirks these changes are intended to eliminate).

I'm more inclined to start again (hey, hey, stop booing!)

Contributor

ivolo commented May 1, 2013

@squaremo thanks for the explanation. we might try our hand at an alternative library with channel APIs exposed. I'll let you know!

Contributor

humanchimp commented May 2, 2013

@squaremo I for one would love to see channels exposed. I think you mean that you would want to do a complete rewrite rather than trying to introduce breaking changes to the current, somewhat cobbled-together module. I concur.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment