Is it useful to always return a promise

Question

I'm using bluebird to design some nodejs api wrapper around an http service. Many of the functions in this wrapper are asynchronous and so it makes a lot of sense to return promises from these implementation.

My colleague has been working on the project for a few days now and interesting pattern is emerging, he is also returning promises from synchronously implemented functions.

Example:

function parseArray(someArray){
    var result;
    // synchronous implementation
    return Promise.resolve(result);           
}

I can see how this could be useful if later on the implementation needs to be made asynchronous, as you wouldn't have to refactor the call sites. I guess it's also nice that all methods are consistently "async", but I'm not sure how awesome that exactly is.

Is this considered a bad practice, are there any reasons why we shouldn't do this ?

This looks uselessly heavy. In case of doubt you could always `cast` the return of the function, I can't see any reason to not provide a directly usable value. — Denys Séguret, Mar 18 '14 at 08:26
It's actually quite a common anti pattern, I think having this question is useful since it's an anti pattern to do this. — Benjamin Gruenbaum, Mar 18 '14 at 08:28
The purpose of promises is to make your code simpler and clearer. If using promises makes you add some useless repeated code in many functions, then you're doing them wrong. — Denys Séguret, Mar 18 '14 at 08:32
Doing this deceives a user of the API into believing that it's implemented asynchronously — RemcoGerlich, Mar 18 '14 at 08:39
I think I did ask this question to get others peoples expertise and 'opinion', Primarily as argumentation fuel for a discussion with my colleague later on, because, clearly, we shouldn't be doing this. Where would have been a more appropriate to ask this question ? — Willem D'Haeseleer, Mar 18 '14 at 08:42
@WillemD'haeseleer this is not opinion at all as far as I can tell. This is an extremely common anti pattern I run into issues I monitor in promise libraries and JS questions all the time. You clearly shouldn't be doing this - (OTOH, if promises were monadic and JS had do notation - that might have been another story with another boxing facility (not promises)). If your coworker has Haskell (or other FP language) experience that might make sense to him but it doesn't work in JS. — Benjamin Gruenbaum, Mar 18 '14 at 08:46

Benjamin Gruenbaum · Accepted Answer · 2014-03-18T08:41:42.607

10

There is no point in returning a promise in synchronous methods.

Promises provide an abstraction over concurrency. When there is no concurrency involved such as when providing an array. Returning a promise makes for worse flow control and is considerably slower.

This is also conveying the wrong message. In fact, promisifying things with no reason is quite a common anti pattern.

One case where it is useful is when a method might be asynchronous - for example: fetching something from cache or making a request for it if it's not there:

function getData(id){
     if(cache.has(id) return Promise.cast(cache.get(id));
     return AsyncService.fetch(id).tap(cache.put);
}

edited Mar 18 '14 at 08:41

answered Mar 18 '14 at 08:27

Benjamin Gruenbaum

270,886
87
504
504

"*considerably slower*"? It should be hardly noticeable with a good promise implementation. – Bergi Mar 18 '14 at 12:07
1

@Bergi than sync execution? Note I'm saying it's considerably slower than sync execution and not form callbacks :) – Benjamin Gruenbaum Mar 18 '14 at 12:08
Yeah, a good promise implementation should be able to handle this as fast as a synchronous execution plus only a constant (and small) overhead. Slower, yes, but not much. – Bergi Mar 18 '14 at 12:13
Here made a (probably broken) micro benchmark http://jsperf.com/return-3-vs-bluebird-promise , unsurprisingly even when the value isn't used and the extra overhead of the `then` call is required (and it does add its own toll) - it's almost 50 times faster. – Benjamin Gruenbaum Mar 18 '14 at 12:13
OK, compared against a single addition the creation of a promise object is much slower. But assuming a non-trivial function (such as `parseArray` like in the OP) the difference shouldn't be that much. – Bergi Mar 18 '14 at 12:15
@Bergi TBH it's more the memory overhead (and GC time it causes) that worries me. If yo're calling something generating a promise 10000 times in a loop that's over 100MB of extra memory I have to use and has to be collected even in the most efficient promise implementation. Do you think I should clarify that in the answer or phrase things a bit differently? – Benjamin Gruenbaum Mar 18 '14 at 12:19
Yeah, maybe. Yet I still think that most operations deal with lots of data, where one promise instance more or less shouldn't make much difference. Have a look at this benchmark for an example: http://jsperf.com/return-3-vs-bluebird-promise/3 – Bergi Mar 18 '14 at 12:48
Interesting. Although in your case both create a promise. Anyway, I think I'll just keep the comments here for reading in case anyone is interested :) Good work on the promise tag by the way - I've been lurking it lately trying to help whenever I can and noticed your positive activity! – Benjamin Gruenbaum Mar 18 '14 at 13:07
@BenjaminGruenbaum In your benchmark the synchronous version is DCEd - [add huge comments like this](http://jsperf.com/return-3-vs-bluebird-promise/4) to prevent inlining (and thus DCE from the loop) – Esailija Mar 18 '14 at 14:07
@Esailija don't unbreak my benchmark - I said I have a broken micro benchmark and I intend for it to remain broken :( . Plus, inlining would occur in the real world scenario too so I don't see what's the harm in it. – Benjamin Gruenbaum Mar 18 '14 at 14:13
@BenjaminGruenbaum you don't see the harm in benchmarking air? wat – Esailija Mar 18 '14 at 14:18
@Esailija I should have realized after this much time that you never get my jokes -_- My bad :P – Benjamin Gruenbaum Mar 18 '14 at 14:28
@BenjaminGruenbaum "When there is no concurrency involved such as when providing an array." What if I am constructing a data structure with several arrays and this data structure is constructed recursively? The array operations (like `push()`) are synchronous operations but would it make sense to design the recursive build-function asynchronously? I am afraid that the backend is blocked in case that many of these constructs are built at the same time due to many array operations during the built-processes of these data structures. – Vegaaaa Jan 09 '18 at 13:39
1

@Vegaaaa no - that's even more clearly a case to avoid asynchronous programming - if you have CPU bound work use a web worker. – Benjamin Gruenbaum Jan 09 '18 at 16:39
@BenjaminGruenbaum so one can say to always avoid asynchronous programming in recursive program flow in JavaScript? – Vegaaaa Jan 10 '18 at 11:16
1

@Vegaaaa no, sometimes it is required - the advice is to merely avoid it when it is not required. – Benjamin Gruenbaum Jan 10 '18 at 12:22

score 3 · Answer 2 · answered Mar 18 '14 at 14:17

3

If we can ignore perf it's only bad if the example implementation is used:

function parseArray(someArray) {
    var result;
    // synchronous implementation
    return Promise.resolve(result);           
}

This function is a mess because it can throw synchronously but also returns a promise. A function that returns a promise must never throw - otherwise you have lost most benefits of promises because now you have to use both try-catch and .catch().

The correct way to implement is to "annotate" a function with .method:

var parseArray = Promise.method(function() { 
    var result;
    //Promise.resolve(result) is unnecessary now
    return result;
});

Now the function is guaranteed to never throw synchronously and can be used in a consistent way.

answered Mar 18 '14 at 14:17

Esailija

138,174
23
272
326

Note that `Promise.method` is really cool and converts thrown exceptions to rejections but is Bluebird specific. – Benjamin Gruenbaum Mar 18 '14 at 14:29
Stating that function that returns promise must never throw, is not perfectly right, e.g. any function should throw if provided arguments are invalid, that's the case with all other async API's, it shouldn't be different with promises. – Mariusz Nowak Mar 19 '14 at 11:21
@MariuszNowak "any function should throw if provided arguments are invalid" I think 'invalid' is a very fussy specification. What if the function needs to do an async call to validate the arguments ? It makes much more sens to me to throw async (reject) if the function has at least 1 async operation. – Willem D'Haeseleer Mar 20 '14 at 10:08
@WillemD'haeseleer invalid on language level, e.g. we expect function and get null, anything that prevents us from initialization of async request. Of course any error result of asynchronous operation surely must be rejected via promise, but it's totally different thing. – Mariusz Nowak Mar 20 '14 at 14:34
1

@MariuszNowak functions that return promises __must never throw__. If they do, it can mess your code up in _oh so many ways_. When you have an array of functions, or you map to a list of functions. In a language without destructors or any sane way to do deterministic resource management - throwing from functions that perform async operations is extremely dangerous. If you want to signal an error from a promise you can _reject_. Otherwise, you need two catch clauses. It also won't work well in chains. – Benjamin Gruenbaum Mar 24 '14 at 01:58
@BenjaminGruenbaum It's about obvious errors that can be picked up just by plain code analysis. Same way as you do not try/catch every sync call, same way you won't try/catch async call, but of course both types of functions will throw if you use them in obviously wrong way. All async API's up to date behave that way, and promise returning api's should not be exception. – Mariusz Nowak Mar 24 '14 at 11:54
If you don't try catch a promise (which, just means - don't append a `.catch(handler` or `.then(..,handler` to it) you get the exact same effect in good promise libraries - it will log (by default) to the console informing you of your error. You can also use typed or predicate based `.catch` clauses in Bluebird. If you allow promise returning function to throw you open [the gates of hell](http://stackoverflow.com/a/21123756/1348195) ( see the analogy :)?) – Benjamin Gruenbaum Mar 24 '14 at 23:48
@BenjaminGruenbaum This article sums it up well http://www.joyent.com/developers/node/design/errors See distinction between operational errors and programmer errors. What I was trying to say is that *programmer errors* should always crash no matter with what type of async handling you work with and promises should not be exception. – Mariusz Nowak Mar 29 '14 at 18:02

Is it useful to always return a promise

2 Answers2

Linked

Related