cujojs / most

Ultra-high performance reactive programming
MIT License
3.49k stars 231 forks source link

awaitPromise() chain should have an timeout failure report option #524

Closed singggum3b closed 5 years ago

singggum3b commented 5 years ago

Summary

Using awaitPromise(), will result in very hard to debug issue when one of the promise in the chain never resolve/reject. It make the whole chain in pending state forever , with no error/failure. Basically the stream is frozen in non dead state. Developer won't know where's the issue coming from

Expected result

Has some default timeout mechanism to report the state of the stream and/or warning of unresolved promise in chain.

Actual Result

Stream is frozen in non dead state.

singggum3b commented 5 years ago

Hi , hope this make sense to you, since it is an issue encountered in actual development usage

davidchase commented 5 years ago

👋 @singggum3b and welcome i would be curious to see a small implementation on when something like this issue occurs. Maybe you can put together something in a online sandbox to showcase the issue you encountered.

IMO this is something that should be left to the user to implement rather than putting on the authors of the library in this case most I think a situation like this could happen regardless of using most or not, for example just using promises or with async/await. How would you handle the potential issue in that situation?

singggum3b commented 5 years ago

i setup a simple fiddle here: https://jsfiddle.net/1b68eLdr/54528/

The problem here i think is awaitPromise utilize promise chain, which is error prone when one promise in the chain is unexpectedly stay in pending state forever.

May be add a documentation note so user will keep that in mind ?

But i think a stream which become undead and can't be revive or detect should not be possible to produce.

davidchase commented 5 years ago

The problem here i think is awaitPromise utilize promise chain, which is error prone when one promise in the chain is unexpectedly stay in pending state forever.

So how would you solve this if you just used promises for example and did not use streams ?

It seems that is more on the level of how do I deal with promises that maybe be pending indefinitely rather than an event stream signaling to the user that it’s value in this case a promise has some kind of issue.

What if the value wasn’t a promise and the same issue you mention above happens ?

briancavalier commented 5 years ago

Hi @singggum3b. I think the root issue here is the forever-pending promise. awaitPromises' job is simply to maintain a strict ordering over the promises it's given.

Since the root issue is forever-pending promises, I think the best way to solve it is at the promise level. Many popular promise libraries, such as bluebird and creed, provide a timeout function that will do what you need, and there are several standalone promise timeout packages, e.g. p-timeout.

Solving it at the promise level with existing functions means that awaitPromises can remain simple and single-purpose. The fact there's an existing solution also means that every library that interacts with promises doesn't need to create its own promise timeout solution.

singggum3b commented 5 years ago

yes, what i mean is that many developer doesn't aware of how awaitPromise work, just like not everyone is on the same level of understanding, so at least i think a note in documentation about this problem is better for everyone ?

briancavalier commented 5 years ago

Agreed. Adding a note to the docs about it sounds helpful. We'll add that soon.