aantron / promise

Light and type-safe binding to JS promises
MIT License
340 stars 24 forks source link

Promise.allOk and its variations #54

Closed user-0a closed 4 years ago

user-0a commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

https://github.com/aantron/promise/blob/ae44bad610e222e6e5d1f6825efd8f4035f68769/src/js/promise.re#L279-L289

user-0a commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

Ahh, thanks for the explanation! I understand now.