Exceptions cause resolution with `undefined`

[jeddeloh]

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 an option. Is this intentional?

Promise.resolved(Ok())
->Promise.map(_ => Js.Exn.raiseError("test"))
->Promise.get(result => {
    Js.log2("I wouldn't expect this to log but it will", result);
    switch (result) {
    | Error(_) => Js.log("and will never get here")
    | Ok(_) => Js.log("ok")
    };
  });

[aantron]

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 Result helpers, to figure out how people use them, and, based on experience, add some kind of catching function that would map caught exceptions to Error(`Exn(e)). You would then use that helper whenever your code can be reasonably expected to raise. Not using it would be a programming error, so maybe it would be fine either reject or to return a forever-pending promise.

As I understood it, the Prometo library was heading toward Error(`Exn(e)) (cc @yawaramin).

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 Lwt.t('a), which is roughly Promise.rejectable('a, exn), whereas rescript-promise is rejectable with any type, whether exceptions or any other values.

[yawaramin]

Hi, yeah, Prometo guarantees that it never rejects. If there's an exception when running the promise, it resolves the promise with Error(`Prometo_error(<Js.Promise.error value>)). But it exposes the standard map, flatMap, etc. API for this wrapped type so you typically don't need to deal with the extra layer manually. That's the 'raison d'etre' of Prometo, basically.

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

[jeddeloh]

Thinking through it, how about making the promise forever-pending?

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 Error result, but that of course requires the polymorphic error variant if you also want typed errors. The alternative, is to both handle the error and catch, but there are no guarantees you'll remember. :) I think bs-fetch has some sort of exception conversion function as well?

We only recently started using `Exn(exn) widely but so far like it. It seems like the only way to get everything we need, but comes with a few downsides as well.

[aantron]

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.

Definitely. At the moment, it does (or should?) at least notify you, when it passes the exception to Promise.onUnhandledException, which, by default, logs the error. Is it doing at least that?

The issues are:

1. How should exception handling combine with promises?

   The suggestion so far is to basically add a function handle: ('a => 'b) => ('a => result('b, [> `Exn(exn)])) so you can easily promote exceptions into results. I don't want to bake that default behavior in everywhere, because that would force every promise to get typed as potentially-throwing (-raising?), which forces handlers everywhere on users. I think there is some benefit to e.g. a user being able to handle errors early, and then, with the type system, being able to guarantee that the remaining promise is free of errors, or else to encode those errors explicitly in the type.

   We could make all promises potentially-raising, actually, and then add some kind of noRaise helper that will "decay" them back to promises typed as non-raising, by internally extracting Error(`Exn(_)) and logging it, or otherwise handling it.

   I should also by now take a really close look at what Async does, since it combines non-failing promises with monitors for an exception-handling mechanism that I've always heard praised.

2. Where exceptions correspond to programming oversights and are not wrapped in handle or otherwise handled, it's not a problem to simply log them. The issue is what to do with chained promises afterwards. Definitely, their callbacks should not be executed.

   • If we reject the promises, that can still trigger a false Promise.Js.catch later on, for a user that is using Promise.Js to get rejectable promises. This seems especially dangerous if someone is using typed rejections (e.g. Promise.Js.t(int, string)) and we suddenly inject a rejection with exn — or any other type, really, since we can't know what type(s) the user is expecting in a rejection.
   • If we leave the promise forever-pending, I think we risk a small memory leak in some cases.

   Dealing with this part is not something you'll have to remember to do — the unhandled exception has already been logged, and the purpose of this second mechanism is just to avoid executing the follow-on callbacks. The only thing to remember will be to handle the exceptions, which onUnhandledException should remind the user of. So hopefully not this... :P

   The alternative, is to both handle the error and catch

[jeddeloh]

At the moment, it does (or should?) at least notify you, when it passes the exception to Promise.onUnhandledException, which, by default, logs the error. Is it doing at least that?

Yes, that's working. It hits onUnhandledException first, and then the next callback gets called with undefined.

[aantron]

The above commit (f139ac348aa1e7b09042747d5e4ffbabefbadea1) 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 onUnhandledException. So it wouldn't be a silent memory leak, but a pretty loud notice of a likely programming error.

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 catch on the JS side that should never have been triggered because of an earlier chain of reason-promises that should not have rejected.

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.

[aantron]

And, thanks again for the report! This was (and until the release, remains) a very serious bug.

[aantron]

The release became reason-promise 1.1.3 and is now available in npm.