Promise.allOk and its variations

[user-0a]

Hi,

Thank you for this great library! I was wondering it would be possible to add Promise.allOk (i.e. a result version of Promise.all) and its variations? At the moment, I believe there is nothing provided that replicates the fail-fast behavior of JavaScript's Promise.all with the result type. Thanks for your consideration!

[aantron]

Yes, we should add this. Thanks for the issue!

Would you like to give it a try? Otherwise, I will do it in some days. I'm on a semi-vacation ATM. Still working, but slower than normal :)

[aantron]

The linked commit added allOk, allOk2...allOk6, and allOkArray, and these functions are now in npm as part of reason-promise@1.1.0. Thanks again for requesting them.

I've linked them from the docs at the bottom of the error handling section.

One side effect is that all the allOk* functions have bloated the compressed size of compiled reason-promise from about 1K to nearly 4K. However, I think I can eliminate most of that in a future release by taking advantage of the fact that tuples are arrays, and simply unsafely defining all the allOk* functions as allOkArray, which is already the case for the plain all* functions and allArray.

[aantron]

Also, the implementation is slightly tricky for the sake of memory safety, see this comment if interested :)

promise/src/js/promise.re

Lines 279 to 289 in ae44bad

	promises |> Array.iteri((index, promise) =>
	/* Because callbacks are added to the user's promises through calls to the
	JS runtime's Promise.race, this function leaks memory if and only if the
	JS runtime's Promise functions leak memory. In particular, if one of the
	promises resolves with Error(_), the callbacks on the other promises
	should be removed. If not done, and long-pending promises are repeatedly
	passed to allOk in a loop, they will gradually accumulate huge lists of
	stale callbacks. This is also true of Promise.race, so we rely on the
	quality of the runtime's Promise.race implementation to proactively
	remove these callbacks. */
	race([promise, callbackRemover])

[user-0a]

Hey, thank you so much for this! Out of curiosity, could it have been implemented with something like this? (in ambiguous terms)

race(all(promises |> map(then(result => <if result is error then reject here using rejecter, return result>))), rejecter)

[aantron]

Not with memory safety. In that pseudocode, the function that is directly called on each promise is then. So if promises is [p1, p2], the code will attach a callback to p1 and the same callback to p2. If p1 resolves with Error(_), the "final" promise of allOk gets resolved with Error(_), but there is no way to eagerly remove the callback on p2 — it can only be removed when p2 also resolves. So that would be a memory leak. The pseudocode might work if promises had some kind of intelligent transitive callback removal scheme, so that fast-fail in race triggered callback removal for everything passed to all, which then triggered callback removal for everything passed to each then. But to my knowledge, no promise implementations do that. I'm not sure if such a scheme could be correct, predictable, performant, etc.

I believe the function that is directly called on p1 and p2 has to be race, because it is the only function that has fail-fast behavior, which, if implemented correctly without memory leaks, should remove callbacks it added to promises that haven't resolved. The only other case in which this happens in JS promises is all in case of rejection, but reason-promise doesn't use rejections, except at the binding level in module Promise.Js, and, anyway, it wouldn't help, because resolving with Error(_) is not a rejection at the JS level :) So that all leaves race as the only choice for a function to call directly on the user's promises.

[user-0a]

Ahh, thanks for the explanation! I understand now.