-
Notifications
You must be signed in to change notification settings - Fork 467
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Non-futures API #9
Comments
For example, this rewritten README example looks good enough for tutorials: var q = 'tasks';
function onError(err) {
console.error(err);
process.exit(1);
}
function publish(conn) {
conn.createChannel(function (err, ch) {
if (err) {
return onError(err);
}
ch.assertQueue(q);
ch.sendToQueue(q, new Buffer('something to do'));
});
}
function consume(conn) {
conn.createChannel(function (err, ch) {
if (err) {
return onError(err);
}
ch.assertQueue(q);
ch.consume(q, function(msg) {
console.log(msg.content.toString());
ch.ack(msg);
});
}
}
// Publisher
var conn = require('amqplib').connect('amqp://localhost', function (err) {
if (err) {
return onError(err);
}
publish(conn);
consume(conn);
}); |
I don't think it would be nicer. Why not? Because for the purpose of the API, promises are strictly superior to using callbacks. For the most part, callbacks are trivially rewritable to promises -- just put the callback(s) you were going to use in the 'then' bit instead of the original function call. When they are not rewritable, it's in best-avoided cases like the one in your example above, in which On the other hand, promises are often not trivially rewritable to callbacks. Since promises are values, they can be handed around for other procedures to synchronise on, and combined in various ways (e.g., wait for all of these). Trying to do this with callbacks just leads to recreating all the same machinery, but many times over. The painful callback-oriented API of node-amqp was one of my motivations for providing an alternative.
There is, mainly in channel.js. It uses promises for RPC, and hence and otherwise very little use of callbacks. That doesn't mean it would be impossible to make a callback-style API on it though. (The result would be promises layered on callbacks layered on promises ..)
Maybe you could ask the reviewer? |
I can think of two things in favour of callbacks:
Actually I don't know if that second one bears out in practice (so I would welcome examples). Otherwise it sounds a bit like "It's the Node.JS Way", which is a dismal argument. The first one is pretty specific, in fact I can't think of a pertinent example in the amqplib client API other than the one I've given. |
Just posting to say I was so glad to find this library and find out it used promises. I was using Q, but for this project I switched to when for max compatibility. Promises really make this thing. That said, it is trivial to support both promises and callbacks, though if you wanna say "screw you guys, I'm not implementing callbacks", I would support that decision. |
@myndzi, thank you for your kind words. I don't want to say "screw you guys". But I do admit to thinking "Callbacks? Really?". I tried to replace some of the uses of promises in the internals, and .. it really just makes things more difficult, and uglier. So I am not inclined to do that. However, it wouldn't be too difficult to provide a callback-oriented public API, possibly with a bit of meta-programming ("unpromisify"?). What is lacking to date is a solid reason to do so. |
Just say you'll accept a pull request ;) An easy and simple way to provide a dual API is simply to use callbacks if a function is supplied (last, node-style, say), and otherwise create a promise and return it. |
I'd suggest having an alternative API, i.e., a separate module. This way if we figure it's not a good idea and does not work very well, it can be removed without painful changes to the other parts of the library. |
I don't like this solution for a couple of reasons. Firstly, I like monomorphic procedures. Secondly, I like "union" return values even less than I like futzing with arguments. (Though there are a couple of places where you'll see counterexamples ..) |
For the most part, the choice between callbacks and promises is one of personal preference, and most developers don't want to be forced to use one or the other. This goes for maintainers and developers. Personally, I dislike using promises as they feel heavier than callbacks. Given the closure-centric nature of javascript/node the more complex mechanisms that promise libraries often provide are fairly easy to implement in node. Node.js' core and many integral modules use the callback paradigm. For consistency with much of the rest of the platform, exposing a callback-based API would be beneficial to many users. Another benefit to using callbacks above the two you outlined is that it requires no external dependencies, and is compatible with virtually all forms of callbacks in Node. Promises are not always as portable, due to the variety of modules that provide similar functionality, like We're currently researching Message Queueing libraries for Node, and are considering your module for use in production. The fact that it's inconsistent with the rest of our system is more offputting than you might imagine. Some of the exposed complexity that callbacks apparently create can be alleviated by adding an internal command queue, bringing the API closer to Streams. Libraries such as var amqp = require('amqplib');
var q = 'tasks';
var conn = amqp.connect('amqp://localhost');
conn.on('connect', function() {
console.log('client connected');
});
// publisher
var publisher = conn.createChannel();
publisher.assertQueue(q);
publisher.sendToQueue(q, new Buffer('something to do'));
publisher.on('open', function() {
console.log('publisher opened');
});
// consumer
var consumer = conn.createChannel();
consumer.assertQueue(q);
consumer.consume(q, function(msg) {
if (msg !== null) {
console.log(msg.content.toString());
consumer.ack(msg);
}
});
consumer.on('open', function() {
console.log('consumer opened');
}); This approach can seem to remove some of the reliability guarantees, but for a production system we simply do not begin accepting connections or work until all auxiliary channels are open. It's true that promises are often not trivially rewritable to callbacks. For some users this could be a problem. Some promise libraries, of course, provide means to easily wrap libraries which provide a callback-based API with promises. Ultimately, though, it is the perceived weight of promises--regardless of whether said weight exists--that will dissuade us from using amqp.node. |
I'm closing this issue as the change has been considered and there is no interest in adding one more API, at least at the moment. |
My proposal is not to add an API, but to switch to callbacks for consistency and use internal command buffers to simplify its usage. And effectively remove the use of callbacks from most of the public-facing API. |
Eli, thanks for your detailed comment. There is some truth (or truthiness, since it's JavaScript) to the claim that promises v callbacks is a matter of taste. It's all just computer, after all. It's also the case that promises just work better here -- not politically, I grant you, but they make for shorter and tidier programs, all else being equal. I have, still in a local branch, a callback-oriented API -- yay! -- made by adapting the promise-oriented API -- oh? -- via Function#apply. As such it's less efficient, though possibly not detectably so. How would you feel about using that? (not a rhetorical question!) And now, please forgive me, I'm going to cherry-pick a few things I didn't get:
Sure, but why would you, if it's already available in the form of a promise library? You're just introducing the chance of getting it wrong.
The first half is certainly fair, but I don't understand the second part of that statement. What does compatibility mean here?
Not sure how that follows. Would you explain?
This is the output from
amqplib has 4 direct runtime dependencies; seems to be pretty par for the course among this lot. Express -- ok it does a lot -- has 11 direct dependencies (and given that, curiously few transitive dependencies). In my experience Node.JS module authors are not especially parsimonious with dependencies. Nonetheless I do agree that it's desirable to be so.
Not sure what you're suggesting here. Complexity for the implementation or for the application? |
As we're hoping to use this in production efficiency is certainly a hope. I'm currently working on modifying amqp.node to work with callbacks and a command queue, for your review.
That's more or less why I built skeggse/nicely. It packages and tests the common callback aggregation paradigms (and other useful constructs) for use in a closure-centric language. This is far more compatible with the rest of the Node core, but provides the useful parts. It leaves you to explicitly handle more complex multilevel constructs as this makes the code more verbose and readable IMO.
I'll try to expand on this point. The two major promise libraries in Node that I know about provide similar functionality. Each provides said functionality via distinct APIs, with different expectations and different mechanisms (apparently I like that word today), and is therefore not necessarily compatible with the other. If I were using two promise-based modules, and each depended on a different promise library, I couldn't interoperate as easily--at least, that's my understanding. The bottom line is that callbacks are pretty much always the same, that's why skeggse/nicely works, whereas promises expose varying methods that may conflict and effectively force the promises world towards a monopoly or users away from promises or your library.
amqplib has 4 direct runtime dependencies and many more indirect runtime dependencies. Some of these are certainly not avoidable, but in the context of some organizations each dependency must be reviewed--both in terms of code and in terms of licensing--before the organization can use the library.
I'm suggesting that this would alleviate the ugliness of the user-facing code, though the internals would still be a mystery. CoffeeScriptAnother little point. This might not be an entirely fair comparison, but I find CoffeeScript detestable. If I want to use a module and it's written in CoffeeScript, I don't. If something goes wrong at 3am and I need to fix it, I can't. It's not worth the risk. Much like CoffeeScript, I find promises overcomplicated and convoluted. It's not just the API I use that comes into question, it's whether I can reasonably fix the problem without understanding the specific flavor of promises in use by the library. |
To @myndzi's point
This is one of the problems I was trying to outline above. He had to switch libraries he was using to use this library. Okay, he didn't have to, but it was for "max compatibility." This more or less means you're dictating which promises library your user uses.
As long as you comply. |
That's actually not supportive of your point, and I'll explain why. For me, it was an open choice whether I used when or Q. I wanted things to look nice (== the same between different code files), so I used when instead. However, I could just as easily be using Q. Both when and Q conform to the Promises/A specification, and they are not only compatible with each other but pretty much any other promise library too. For best effect, you would wrap promises in the library you're using, and it would conform them to its own api, but the incompatible bits you are talking about are basically sugar. For example, when uses .otherwise while q uses .catch or .fail. Both libraries, however, accept the syntax Constructor(fn|value, success, failure). Both libraries can also accept and work with jQuery promises, which are not Promises/A compliant to begin with, or pretty much anything with an acceptable 'then' method. So, when I write compatibility there, I'm really only talking about visual consistency. Which, I admit, can certainly influence decisions! But it's not as big a thing as you infer. |
That's good to know, thanks! You're right, though. Consistency does factor into this a lot for us. It helps keep larger organizations and projects unified, for the same reason that some projects include style guidelines. |
Not really, there's three indirect runtime dependencies (two for readable-stream, and one for bitsyntax). It's mocha and instanbul that have all the deps, and they're devDependencies.
It's not the case though. As @myndzi explained, promise libraries conform to Promises/A+, and are thereby interchangeable.
That's what I was talking about. You're substituting an ad-hoc set of procedures for a commonly understood (standardised, even) abstraction. Hence, introducing new terms and uncertain semantics to figure out. |
I'd like to point out that futures (what is called promises in JS land) are used in multiple other languages (primarily JVM and .NET-based ones) and have stood their test of time. Composability and interoperability of implementations Replacing a well known (at least in some areas of our industry) abstraction with your own ad-hoc "internal queue" (whatever that means) sounds like a step backwards to me, even if it's mostly the case that Node developers In any case, my request was considered, and there is no consensus, so things better stay the way they are for now. |
The code example @skeggse added above demonstrates it: it just means operations are made synchronous. amqplib already does this (it's a de-facto requirement of AMQP), so one can write code that looks much the same as the above as things stand. |
@squaremo thanks for the clarification. It makes sense for many operations in the protocol and most clients do the same (but |
To clarify on that point, it does not mean the commands are synchronous. Rather, commands like At the very least you all have helped me understand promises and their uses better. I did some research I should've done years ago. It's not simply callbacks vs promises, it's imperative vs functional. Callbacks are imperative, promises are functional. Instead of telling Node how you want something done, you tell it what you want it to do. Similarly, the command queue lets you tell the library what you want it to do without caring exactly how it gets done. I would argue that, as the article pointed out, Node is at its core imperative--event emitters notwithstanding--and therefore callbacks suit the framework better than promises. That said, there is always room for improvement. However, every abstraction comes at a price. I ran a few tests for performance and found that callbacks appear to be more memory, cpu, and time-efficient. I didn't trust those results, though, as they felt rather poorly constructed. Somebody else formalized the process and found that promises, depending on the framework, are more expensive than callbacks. I wouldn't base the decision on that, though, as for many developers that level of performance is irrelevant. Still, given that message queues are often used to increase the efficiency of a distributed system, performance can be a sticking point. On a more practical level, I still think that consistency with the rest of the Node platform is important. Also, it isn't just as simple as adding a .then to your API calls. Callbacks passed to then get one argument, representing the value of the completed operation. Node callbacks get a function signature like I certainly wouldn't use skeggse/nicely for a shared library, but it has become the defactor method for working with asynchronicity for us--that was my point. In the end, as a the maintainer, it's your decision. I will still make a true callback-based version of the library, so we can maintain it and fix it for production. |
What I ought to have said is that it serialises operations by synchronising on the replies. The effect is that code looks synchronous until the last operation, when you supply a callback (i.e., do your own explicit synchronisation).
James' post is pretty good I think (it was a shame he recanted in a follow-up). You are quite right about there being a trade-off. In general, promises will involve more cons'ing and processing. Although they may be more efficient in some ways, since resolutions in an event loop are (at least in when.js) batched, whereas callbacks may each be scheduled separately. But, let's say, on balance promises are a bit more work for the computer. There are two mitigating factors in the case of amqplib:
On error handling:
Unless I'm mistaking your point, this is absolutely possible with promises. Say I have two operations, creating an anonymous queue and consuming from it. I have different error continuations for each operation (for simplicity's sake, just logging a warning). var ok = ch.assertQueue('', {durable: false});
ok = ok.then(function(q) {
return ch.consume(q.queue, handleMessages, {});
}, function() {
console.warn('Failed to create queue');
});
ok = ok.then(nowGoAndDoSomethingElse, function() {
console.warn('Failed to consume from queue');
}); Anyway. It occurs to me to mention that I recently changed the underlying channel machinery to use callbacks for RPC. It's only the API that layers promises on top. If you wanted to test whether promises make an appreciable difference to performance, you could use, e.g., ch._rpc(defs.QueueDeclare, {queue: 'foobar'}, defs.QueueDeclareOk, function(err, ok) {
console.log('Queue declared: %s', ok.fields.queue);
}); In order: the method you want to send, all its arguments (I've missed some, check http://www.rabbitmq.com/amqp-0-9-1-reference.html#class.queue), the method you expect in reply. The callback gets the whole frame, so to get a field value you'd dereference |
Network latency will likely dwarf differences in latency, but if you're talking throughput may still have an impact.
I started noticing that eventually, and though there are still parts which uses promises--the handshake, for example--those are of infrequent use and therefore the performance drops are virtually nonexistant. Error Handling
My point is that, if we're using nicely or something that makes assumptions about the signature of a callback, we can't continue to drop in our old callbacks, but have to create intermediary proxies for the other callbacks. Here's a simple example. Trivial in the exact case, but if var rimraf = require('rimraf');
function majorWreckage(err, queue) {
if (err)
// probably not a good failure-strategy
return rimraf('/', console.error.bind(console, 'major wreckage in response to', err.stack || err));
ch.consume(q.queue, handleMessages, {});
}
var ok = ch.assertQueue('', {durable: false})
// with callbacks I can just pass majorWreckage and the function gets called with an error or with a value
.then(function(q) {
majorWreckage(null, q);
}, function(err) {
majorWreckage(err);
})
.then(nowGoAndDoSomethingElse,
console.warn.bind(console, 'failed to consume from queue')); While yes, I can rework majorWreckage into two separate functions, I don't always have the power to modify the function I provide. |
This can be a bit tidier if you use
but yes, if what you have is a |
My point was I can't just drop in |
Well, you're using two different interfaces in the same code. When you do that, you have to create an intermediary. It doesn't make promises 'bad', and a barrier to adoption is people avoiding them for reasons just like this. The existing promise libraries provide methods to automatically wrap node-style callbacks into promise-style functions, use them. |
There's now a callback-oriented API in master branch: An observation: some modest testing indicates that it's no faster than the promises API, but then, such tests mostly send or receive messages and that uses exactly the same machinery, in the same way, as the promise API. Another observation: there is an irritating amount of almost the same but not quite code between the callback and promise APIs. I've factored out a fair chunk of it but some remains. |
@squaremo yay! Excellent job! |
I wanted to thank all of you you for helping me think logically about promises v callbacks. Promises are a really cool abstraction. I still can't quite get to the place for whatever reason to like them aesthetically, and I still have these weird reservations about efficiency which is a moot point. That all said they're a useful construct, and they do add a lot of elegance to amqp.node's API. Thanks for adding the callback API, it means that amqp.node is now more in-line with Node's asynchronous paradigms, but that may end up changing if Node adds promises to its core (didn't I hear one of the core developers talking about that at one point?). Anywho, awesome work! 😄 |
Thanks @skeggse! |
More feedback from an experienced Node.js person: it would be nicer if
amqp.node
could provide a non-futures-based API. I recall there is a "lower-level API" but not sure if it is callbacks-based or not.Then people who want to use futures can do that on top of callbacks. I don't see why
anyone would want to do that but have to trust the reviewer on this.
Can we maintain both "lower-level" API and an opinionated futures-based one? I'd certainly offer a hand if that's possible. Bunny 0.9+ does this to some extent.
This is an important choice because we will (I hope) use
amqp.node
in the rabbitmq.com tutorials.The text was updated successfully, but these errors were encountered: