gladiusjs / gladius-core

Core library for Gladius
BSD 3-Clause "New" or "Revised" License
446 stars 48 forks source link

Refactor asynchronous code to use promises instead of callbacks #127

Open modeswitch opened 12 years ago

modeswitch commented 12 years ago

Callbacks, especially in the get/loader code, makes the code hard to follow. Should we move toward using promises instead?

DamonOehlman commented 12 years ago

I think using callbacks is fine, but I would definitely encourage adopting some of the patterns as generally implemented in node. This would enable the use of libraries such as @caolan's async which in general make async code that much nicer to work with.

I notice the code in question is a call to engine.core.resource.get which I'm guessing is designed to retrieve multiple resources and fire the oncomplete callback when all resources are available. What I'd suggest is perhaps changing the signature of the engine.core.resource.get function to simple be something like:

function(resource, callback) {
}

And as per node general practice have the callback fire with two parameters (err, resource). This could easily be passed to something like async.map to load all of the resources in the same way. I'd personally feel very comfortable with something like this. I am, however, just one person.

If you do decide to go with a promises approach, though, then using something like @briancavalier's when.js library would be an awesome choice. The implementation of when.all is very nice and works really well.

dmose commented 12 years ago

Damon, appreciate the thoughts, these are super helpful. We'll have to take a look at when.js and async. For the promises stuff, we've also been hearing good things about the q library.

briancavalier commented 12 years ago

Hey guys,

Admittedly, I don't know much about the specific situations in gladius, but maybe I can offer some general perspective on promises and when.js. In general, I think promises provide a good foundation for async: they're fairly simple and lightweight, but they make it easy to build higher level async constructs. For example, when'js's when.map and when.reduce are suprisingly compact, but quite powerful. I see that as an advantage of promises, they are both useful in and of themselves, and also make good building blocks.

I also feel that promises make for very clean API design. There is no need for callbacks (and errorbacks, and finallybacks? etc.) to be added to every function signature in your API. You can just return a promise instead. I wrote a couple blog posts a while back about the programming model of promises vs. callbacks: part 1, and part 2

More and more frameworks are integrating promises as well. Dojo and jQuery both provide promise implementations (when.js can interoperate with both, see here). @dmose, when.js was somewhat influenced by Q, which is also a nice, well-thought-out promise library.

I just took a look at async.map(), and when.map(), and it looks like either one would be well-suited to loading a set of resources. The difference, of course, being that async.map() uses a callback style, and when.map() returns a promise.

I don't know if @caolan's async lib provides a similar building block for composing new async operations. It may ... it looks like a very nice library. I think either when.js or async.js would give you some good async tools.

Of course, I'm biased, so I think promises are a better direction :)

caolan commented 12 years ago

Like @briancavalier I'm not familiar with the particular issues gladius needs to tackle, but I'll give you my high-level thoughts on this.

The only compelling advantage of promises I've seen is the ability to add some higher-level visualisation and monitoring tools for asynchronous tasks - this has mostly been demos, I'd love to see someone using these kind of tools in real development. There are some rare occasions where promises make more sense in terms of composition but in the vast majority of cases you can pass around a continuation or callback as easily as a promise and with less boiler-plate. I personally feel that there is a huge advantage to sticking with something that has become intuitive to most JS developers. If you write JavaScript your're used to using callbacks!

Finally, sticking to the Node.js style means you can use a huge number of projects on top of that callback pattern. There's obviously 'async', but a host of other promise libraries etc that can layer on top of a Node.js-style API. Of course, being able to use multiple async styles isn't very useful when you're trying to keep a project coherent, but other projects using your API will probably thank you ;)

Note that I'm a fan of more functional-style code and promises may look more natural to those working in a more heavily OO environment.

caolan commented 12 years ago

RE:

I don't know if @caolan's async lib provides a similar building block for composing new async operations. It may ... it looks like a very nice library.

The building block is a function following the Node.js callback style. It makes sense to me to use the natural features of the language to their full-extent. My code usually includes lots of smaller functions which compose async tasks and which themselves can be handled by the async library. I find this makes the code much easier to understand despite the complex asynchronous operations that may underpin it. I rarely have a function that nests more than one or two async callbacks.

modeswitch commented 12 years ago

The specific cases where I'm forced to deal with asynchronous control flow are in resource loading and interacting with the thread pool.

There are a few reasons why I'm considering promises. One of my concerns is that while asynchronous control flow is natural for JS developers, it might not be so for game developers who are coming from other languages. Ideally, it would be great to appeal to JS developers who want to make games and to game developers who want to make games in JS. (@caolan I suspect a lot of game devs are used to OO environments.) Another side-effect of using promises is that code indentation doesn't get ridiculous when chaining lots of callbacks.

caolan commented 12 years ago

I think using promises because game devs are more used to passing around objects may be a legitimate reason (are game devs used to promises?) but doing it to avoid code indentation is a misconception. Ultimately, you'll be using anonymous functions with your promises too and there is absolutely nothing stopping you flattening the structure of code using regular callbacks in the same way. Usually deeply nested code is just a sign that you are trying to do too much at once. More, smaller functions are the key to all async programming IMHO, promises or not.

AdrianRossouw commented 12 years ago

I find that promises and async are not mutually exclusive, and have different areas where they shine. Promises are basically event emitters with some added semantics to make them useful for state. They allow you to have multiple callbacks for a single event/thing more easily too.

they are useful for situations where you have to start an asynchronous task in one point, and then at a completely other point schedule something to run only once the task has been completed. A good example here is like in connect style middleware. When loading a record to generate a page, you do not necessarily want or need to hold off on calling the rest of the middleware stack, so you could do:

// just starts the loading
function myLoaderMiddleware(req, res, next) {
     req.model = (new Model({id: req.params.id })).fetch();
     next();
}

/* lots of stuff can happen here */

// we need to do something that requires the req.model to already be loaded
function middlewareRequiringModel(req, res, next) {
    $.when(req.model).then(function(model) {
       // model is now sure to be populated here.

    });    
 }
caolan commented 12 years ago

@Vertice that's a good point! I do use EventEmitters + state occasionally with very complex build systems and that is essentially a Promise object. This tends to occur in systems where I have many unknown (at the time of development) steps/tasks defining dependencies then automatically running things in the most parallel way possible. See Kanso for examples of a system that uses code like this: http://kan.so

Of course, the majority of the code is standard Node-style callbacks and only the complex build system uses EventEmitters in this way. The point being that both of these are immediately obvious to Node developers.

briancavalier commented 12 years ago

Great discussion! I started writing a few more comments and it got kinda long, but here's some followup on some of the things so far.

OO vs. functional

My background is OO, and I don't feel there's anything particularly OO or functional about promises. The fact that Promises/A defines them as an object with a .then() API is mostly so that different promise implementations can interoperate easily. I tend to think of them as either completely opaque, or as if they are the value they represent.

I also tend to treat promises just like non-promises most of the time, passing them around and returning them as if they were just regular values. Then, only at the point of needing the actual result do I end up calling when(). More on this below.

When consuming a promise's value, you can use either .then() if you prefer an OO style, or when() if you prefer functional style. Although, IMHO, when() is a better choice for several reasons

Mixing promises and callbacks

I think as long as you use each one where it makes sense, and keep the number of different callback signatures in check, it's fine to mix promises and non-promise callback styles.

Indentation

I agree with @caolan that there's nothing inherent about promises that will magically reduce indentation and nesting. I think it has more to do with personal coding style. For example, if you're diligent about hoisting functions, you'll almost always end up with less indentation.

That said, there are two things that may encourage (but certainly don't guarantee!) less nesting:

  1. Taking advantage of promise forwarding and using calls to .then() at the same level rather than nesting.
// Promise pipeline. Non-nested calls using a promise chain to
// forward the result of one computation to another.  This is a
// totally contrived example, but you get the idea.
when(getResult1())
    .then(computeResult2)
    .then(computeResult3)
    .then(displayResult);
  1. Taking advantage of return. In a pure continuation passing model, you must pass callbacks down the stack, and (most of the time) you can't take advantage of return values. Since promises can be returned, you can pass them back up the stack. IMHO, this encourages regular call-and-return style functions rather than deeply nested higher-order functions that all accept callbacks. I wrote a bit more about this in my Async Programming: Part 2 blog post, linked above.

    Errors can also propagate back up the stack, similar to exceptions and try/catch. In fact, you can throw from inside a promise callback, and when.js will translate that into a promise rejection so that callers higher in the call stack can observe the failure--again, a familiar model that is very much like exceptions.

Function signatures

Big Disclaimer: I am not a heavy node user. To my eyes, though, the "standard" node callback signature is a bit strange and artificial for a couple reasons:

  1. Conceptually, I feel that the first argument to a function should be the primary input to the function--i.e. it should be the thing the function is most likely to use in computing a result.

    The fact that node puts the err param first seems like it forces people to either: 1) write lots of artificial function signatures that can't really be reused easily (except with other node-compliant continuation passing), or 2) write meaningful function signatures, but always wrap them in a node-style callback when using them with async operations.

    That seems to me like it can encourage more nesting, and less reuse.

  2. Using a single callback to handle both the success and error cases means all your callbacks are probably doing double duty, and will often (always?) include an if/then to check err. To me this seems to hinder separation of concerns and reuse.

Again, these are based on my naive view of node callbacks, and in practice these things may not be true at all.

Promises/A promise callbacks separate the success callback from the error callback, and gets rid of the need for the extra if/then. The success callback will always receive the promise value as it's only parameter, and the error callback will always receive the error (aka "reason") as it's only parameter.

In my opinion, that makes for fewer artificial function signatures, and allows reuse of existing functions more easily and without wrapping.

There are, of course, situations where a single handler makes more sense, but most of the time, I find that separate handlers are what I really want.

Other interesting things about promises

The first two were noted by @Vertice, but promises really go beyond just being a standardization of callbacks:

  1. Multiple consumers - A single promise can be given out to any number of consumers. This can be useful in a variety of situations, and thinking of promises as one kind of event emitter is certainly one of those.
  2. Separation of concerns - The ability to resolve/reject a promise can be given out separately from the promise. This allows for some interesting communication models where the producer and consumers don't need to know about each other, and can't interfere with one another.
  3. Immutability and safety - Because of 1 and 2, and because promises are immutable, promises can make strong guarantees. For example, you can give a single promise to multiple consumers and they cannot interfere with one another via the promise. One consumer cannot alter the value of the promise to mess up another consumer. One consumer cannot throw an exception that will cause other consumers not to execute because the promise infrastructure handles exceptions.

Granted, you could build these things into a callback system, but I wanted to point out that they are there in when.js, and in any solid Promises/A implementation.

dmose commented 12 years ago

Thanks for the in-depth analysis, guys. I have to admit, Brian's words and blog posts are very persuasive here. FWIW, the node signature style feels weird to me too, but that's just because I haven't spent much time in node-land, I suppose.

It'd be interesting to whip up a prototype of our loader using promises and then a few game-like tests that act as consumers to get a feel for them...

AdrianRossouw commented 12 years ago

@briancavalier Regarding your points about how node.js callbacks work, you should really spend some time to get acquainted with @caolan 's async.js library. It makes both of those points non-issues by delegating errors to a 'final' function that is always the last argument in the function.

async.partial is also a thing of beauty when it comes to re-using functions. Check the second example in this gist

modeswitch commented 12 years ago

@briancavalier The promise pipeline pattern you used is what I was referring to above, thanks for making that explicit. But you're right, it doesn't guarantee nicer-looking code.

@dmose I don't think it would be too hard to modify the loader to work with when.js. Right now the API is like this:

get([
  { 
    type: Mesh, 
    url: "assets/my-mesh", 
    load: defaultLoader, 
    onsuccess: function( meshInstance ) {
      // stash meshInstance
    },
    onfailure: fail
  },
  {
    type: Material,
    url: "scripts/proceduralMaterial.js?color=red",
    load: proceduralLoader,
    onsuccess: function( materialInstance ){
      // stash materialInstance
    },
    onfailure: fail
  }],
  {
    oncomplete: doSomeWork
    onprogress: updateProgressBar
  }
)

Using promises instead, the API could be something like this:

when.all(
  [get( Mesh, "assets/my-mesh", remoteLoader ), 
    get( Material, "scripts/proceduralMaterial.js?color=red", proceduralLoader )],
  doSomeWork,
  fail,
  updateProgressBar
);

Most of the time (maybe even all the time?) the onsuccess handler will just populate a resource cache with the things we've just constructed. This also sets us up to be able to use generators to yield control until the resources are loaded and resume where we left off (à la task.js).

mikeal commented 12 years ago

I'll try to share a few lessons we've learned in node.js.

1) Forcing people to acknowledge errors as early as often leads is a very good practice. 2) Using a standard callback API is more flexible than promises and you can, in fact, layer promises very easily on top of it.

In the early days node.js has promises and ripped them out before 0.2. It was impressive to see the amount of boilerplate this actually reduced in people's code.

In node.js we use the standard callback interface.

function (error, result) {}

Also, the callback MUST be the last argument passed to a function.

Technically the spec states that you can have more than one result but nowhere in node core does this exist and many promise and deferred abstractions now assume a single success object. This is important because handling an arbitrary number of arguments means you'll end up doing an Array splice on arguments which is actually quite costly in v8 (we had a bit of code that did this in event emitters and removing it showed a 20% performance increase across node.js).

I would encourage front end libraries to adopt this pattern. It's simple, easy to build on top of as many libraries such as Q have shown, and it's easily integrated in to node libraries which seem to be making their way in to front end development.

You also avoid the problem of promise/deferred bikeshedding. There are several flavors and preferred APIs around this problem, adopting one severely limits the higher level implementation of another. The callback interface avoids this and promise libraries can either return a function matching this API or wrap them.

mikeal commented 12 years ago

I'd also warn against adopting APIs that limit your integration with other libraries.

Callbacks are just functions and you can't write a library that doesn't have some level of integration possible with functions. Any higher level API/Interface pattern you layer on top will require adoption across the stack.

This is an important marker in library and API design philosophy. Do you vertically or horizontally integrate the components of your stack?

The biggest proponents of vertical integration (think Dojo and YUI) offer very compelling value out of the box but where they fail is to integrate with the rest of the value being created in the broader community.

Horizontal integration on the other hand creates small components that don't require deep integration in to libraries written by others (think jQuery, Backbone, underscore).

Adopting when.js, or Q, or some other promise library means that libraries written by other people will be harder to integrate unless they've also adopted this abstraction.

Callbacks do not require or limit integration with other libraries unless they found some way of making javascript not use functions. If "managing callbacks" becomes a problem there are litany of libraries to deal with this problem (async.js among them) and these libraries do not perpetuate their way in to the stack and limit integration with the rest of the world.

dmose commented 12 years ago

mikael, thanks for the input. That's a very good point about horizontal vs. vertical integration. It seems pretty clear to me that the adoption characteristics of things that offer horizontal integration tend to be stronger than than those that offer vertical integration, enough other things being equal. Your post also made me a perceive the value of the callback argument ordering in node: it acts as an affordance to force the developer to explicitly make a decision to not supply an errorback rather than making not supplying one the path of least resistance, which I would imagine would lead to higher quality code.

jrburke commented 12 years ago

Some thoughts on the node style:

While it may have made writing the core libraries easier, it just moved the problem the end developer. The function (err, result) {} choice did not fix the issue, it just pushed it off. I believe Node's removal of their promise implementation was more about inexperience, the newness of how best to go about it, both for node and for promise APIs. And that was the right choice for that problem at that time. It allowed them to move on to other decisions.

There are many async control flow libraries for node now. That points to the problem domain being understood better now, and it actually being an issue. Since there is no good built in choice, it is easy to make a library that caters to small, surface style preferences.

The node list gets posts every so often about considering more sync-based solutions with fibers and such. I attribute that to async just being harder, but also the inside-out structure of code that happens with node's style -- any time you have a conditional that may or may not use an async call, you have to pull that code out as a function above the current control flow. It makes the code harder to follow.

I also do not believe this is a horizontal/vertical integration choice. It is about codifying patterns for async programming, and "do you think it is done" choice.

jQuery, Dojo, Q, when, they are all converging on the same patterns around this. It may not be at an end state yet, but it is getting close enough that there are possibilities for overlapping use without needing everyone to use the same promise library. It is also possible to do a lightweight, manual construction of an object that meets either the .resolve()/reject() pattern for callbacks or for objects to return a then()/fail() for control flow. It is taking some time, but it is being settled as it should -- through implementation and use, and hopefully by implementers talking and sharing.

If you can live with the inside-out style of function(err, result){} and you think there still needs to be more time for people to dabble in async control flow libraries, then go that route. Also, if the code is about streams or something that generate multiple events, promises are not a good choice. Otherwise, consider promises, particularly if it is about getting basic "do this, then that" control flow in an async environment.

mikeal commented 12 years ago

@jburke the amount of people actually using fibers, streamline and other "sync" style structures is about .01% of the people using node.js. The mailing list is not a good representation of usage on this issue, the authors of a couple libraries reply to threads like it's their job and over represent their POV on the list. I wrote an article that examined this in greater detail.

Don't really agree with your point about things being inside-out, most node.js code uses inline functions which doesn't suffer this issue.

jQuery, Dojo and Q are not converging on the same pattern. They have the same ideas but are not adopting entirely compatible patterns and API.

If the problem is as you put it: "about codifying patterns for async programming" then it would makes sense to adopt the most widely used js pattern for async, the one used by node.js. Code in node.js deals with async more than any client side js code and while the prominent client side patterns are interesting I think that node has proven at this point that its patterns are fit for complicated applications that deal with a lot of async.

Part of the philosophy of node.js is that things that happen in the future should look like they happen in the future. That abstractions which remove the look and feel of happening later lead to more mistakes and unplanned state mutations. This philosophy seems to be working, node.js adoption is a rocket ship and none of the popular flow control libraries are "promise" based but instead provide API that doesn't lose the look and feel of being code that executes in the future being in the future.

jrburke commented 12 years ago

@mikeal: understood on the fibers-specific thing, there are still questions on how best to deal with async. Everyone new to async struggles with the same issues.

Inline functions only work if there are no conditionals that may have already been satisfied. Example: http://groups.google.com/group/q-continuum/browse_thread/thread/6cc134edcc964013#msg_7d4167a5c9f451f1

If you know of a better way to do that though, I'm happy to know about it. I could just be thinking about it wrong.

Right, the jQuery, Dojo, Q, when, have some differences, but there is enough commonality to be useful -- more examples for front end code that have found a common pattern. To me, that is encouraging when people in different communities come to a same area of agreement. But I agree it is not a completely solved question.

Node is great, growing, and I like to use it, but it is not done either. Some parts are more done than others, and you may feel like this is a done area for node, but I do not see it. The introduction of domains in node indicates to me there is still some work to be done in this area.

async.js is built on the node callback style, something a front end library cannot depend on being implemented, the historical culture is not that way particularly given DOM/BOM APIs, and asyncjs focused more on batch operations. It is fine if you buy into node's callback system, but its series do not look like an improvement over then().

I do not understand the thing about "things should look like they happen in the future". then() seems to convey that fine in promises. Maybe you mean that some promise implementations, like Dojo, may not execute the then() in the next tick, but may do it in the same tick. I agree the best behavior is to wait for the next tick, but fortunately that can easily be fixed in an implementation, the promise API does not have to change.

End result, Node has done some exploration in this area. Others, including folks outside of JS, have done some exploration using promises. Neither have it solved completely. The choice is probably more of a cultural one. Node's culture is not front end culture, but hopefully they share a lot. In any case, I do not think I can provide any more useful feedback, so I'll stop.

mikeal commented 12 years ago

@jrburke your point on conditionals is valid. I have a few of them in my code base but they count for a little under 5% of the callbacks overall. honestly, when i see them I start to wonder if it's time for that code to use an abstraction like async but i resist the urge and ship it anyway :)

You have a valid point about async requiring node's callback pattern but this would be true if you adopted anyone's promise pattern as well. The difference is that integrating a promise library with the callback pattern is very easy, you just put a method on the promise that returns a function conforming to the callback interface. But, making one promise library work with another is a lot more work.

I find it encouraging that libraries like Q have a bit of a following in node. It means our replacement of promises for callbacks opened the door to other libraries solving this problem in their own way.

drewish commented 11 years ago

Sounds like the predicted convergence on Promises has finally happend: http://infrequently.org/2013/06/sfuturepromiseg/

I'm curious to see if this leads to more adoption in the node world.