-
-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Exceptions cause resolution with undefined
#70
Comments
That's a bug, thanks! Thinking through it, how about making the promise forever-pending? I'm concerned about introducing rejections. However, I'm also concerned that using a forever-pending promise would introduce a memory leak (of these pending promises). A rejection might be nicer since, at least in Node, it would cause a message when inevitably not handled, and that message would "spiritually" correspond to there having been an unhandled exception. The original plan was, after adding As I understood it, the Prometo library was heading toward I don't think the community ever really settled on a preference for this, and now improved rejecting promises are being proposed for ReScript (note the proposal changed during the thread). There would still be a difference between rejecting in reason-promise in case of exceptions like in this issue, and what is being proposed in rescript-promise — rejecting only with exceptions is like |
Hi, yeah, Prometo guarantees that it never rejects. If there's an exception when running the promise, it resolves the promise with EDIT: for the question of wrapping promise+result, this thread is apropos: https://forum.rescript-lang.org/t/promise-of-result-why-or-why-not/979 |
I think it's it's important that it reject or resolve with an error in some way. It can be critical that you handle the failure of a promise. Ideally you could guarantee some sort of We only recently started using |
Definitely. At the moment, it does (or should?) at least notify you, when it passes the exception to The issues are:
|
Yes, that's working. It hits |
The above commit (f139ac3) makes reason-promise return forever-pending promises upon unhandled exceptions. I added tests, which failed, and the code fixes them. Returning forever-pending promises can lead to memory leaks in some cases, in code that generates many of these doomed promises and retains references to them. This should be relatively rare, since most code that generates promises in a loop also tends to wait on its previous promises to resolve first, at worst in groups of N. In any case, even if it does occur, it will always be accompanied by a huge amount of spam of stack traces from The other major option, to reject promises, seems like a bad idea to me, since it will introduce rejections where they are not expected, and undermine assumptions about control flow with reason-promise. It seems like it will eventually cause the same issue as here, except in some I am still looking for good error-handling strategies on the higher levels of reason-promise. I'll take care of a few other issues and release this in reason-promise 1.2.0. |
And, thanks again for the report! This was (and until the release, remains) a very serious bug. |
The release became reason-promise 1.1.3 and is now available in npm. |
When an exception occurs in a reason-promise, it resolves the promise with
undefined
. My expectation would be that it either rejects or the type for a result should always be wrapped in anoption
. Is this intentional?The text was updated successfully, but these errors were encountered: