Exception in callback functions

[scull7]

Currently, when an exception occurs within a callback function; handling that exception is very difficult.

Here are several examples listed (imho) in order of severity (greatest -> least)

1. An exception occurs in a callback handler after transforming a Promise to a Future.t. This is particularly insidious because it effectively squelches the error; unless the user has the unhandled-rejection process handler defined.

https://github.com/scull7/future/blob/d055f04fe972e9895d0f6ace2322ff44d0854287/tests/TestFutureJs.re#L101-L121

  testAsync("fromPromise (exception in callback)", done_ => {

    let expected = "TestFutureJs.TestError,2,confused!";
    let nodeFn = (callback) => callback(TestError("confused!"));
    let promise = Js.Promise.make((~resolve as _, ~reject) => {
      nodeFn((err) => reject(. err));
    });
    let future = () => FutureJs.fromPromise(promise, errorTransformer);
    
    Future.value(Belt.Result.Ok("ignored"))
    |. Future.tap(_ => Js.log("huh? - 1"))
    |. Future.mapOk(_ => raise(TestError("oh the noes!")))
    |. Future.tap(_ => Js.log("huh? - 2"))
    |. Future.get(r => {
      switch(r) {
      | Belt.Result.Ok(_) => raise(TestError("shouldn't be possible"))
      | Belt.Result.Error((s)) => s |. equals(expected)
      };
      done_();
    });
});

2. An exception occurs in a callback handler after an asynchronous operation. This causes problems because the exception cannot be caught by the traditional try/catch mechanism. Wrapping the call in a Promise may be a solution, though probably a bad one.

https://github.com/scull7/future/blob/d055f04fe972e9895d0f6ace2322ff44d0854287/tests/TestFuture.re#L126-L142

  testAsync("mapOk (async exception)", done_ => {
    delay(25, () => Belt.Result.Ok("ignored"))
    |. Future.tap(_ => Js.log("mapOk-async-exception-1"))
    |. Future.mapOk(_ => raise(TestError("boom, goes the dynamite!")))
    |. Future.tap(_ => Js.log("mapOk-async-exception-2"))
    |. Future.get(r => {
      Js.log("mapOk-async-exception-3");
      switch (r) {
      | Ok(_) => raise(TestError("shouldn't be possible"));
      | Error(TestError(s)) =>
        Js.log("mapOk-async-exception-4");
        s |. equals("boom, goes the dynamite!");
        done_();
      | Error(e) => raise(TestError(Js.String.make @@ e));
      }
    })
  });

3. An exception occurs in a callback handler after a synchronous operation. This exception can be caught by a traditional try/catch, however, the behavior is surprising, at least it was to me.

https://github.com/scull7/future/blob/d055f04fe972e9895d0f6ace2322ff44d0854287/tests/TestFuture.re#L114-L123

  test("mapOk (sync exception)", () => {
    Belt.Result.Ok("ignored")
    |. Future.value
    |. Future.mapOk(_ => raise(TestError("boom, goes the dynamite!")))
    |. Future.get(r => switch (r) {
      | Ok(_) => raise(TestError("shouldn't be possible"))
      | Error(TestError(s)) => s |. equals("boom, goes the dynamite!")
      | Error(e) => raise(TestError(Js.String.make @@ e))
    })
  });

[gilbert]

For (1), newer versions of node should report uncaught rejections (for the record; in chat you mentioned how jest / ospec were squelching it somehow).

(2) and (3) are due to Future's decision to delegate error handling to the user. The solution is to wrap your code in a try/catch but within the mapOk itself. That way you can convert the error to a more appropriate type.

At least, that's what I think. Are these suggestions missing anything?

[scull7]

I think that covers why the current behavior exists. Perhaps it is best solved with documentation then? The behavior (especially (1)) was very surprising to me.