From 232d81e1a7d8d2b656b0481d3f15125d53b000e2 Mon Sep 17 00:00:00 2001 From: Nathan Sculli <scull7@gmail.com> Date: Sun, 10 Jun 2018 18:31:23 -0700 Subject: [PATCH 1/2] Exception raised in callback is not handled. I'm not sure how this should be handled? --- tests/TestFuture.re | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/TestFuture.re b/tests/TestFuture.re index a184554..5de7dd2 100644 --- a/tests/TestFuture.re +++ b/tests/TestFuture.re @@ -111,6 +111,17 @@ describe("Future Belt.Result", () => { }); }); + test("mapOk", () => { + 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)) + }) + }); + test("mapError", () => { Belt.Result.Ok("three") |. Future.value From d055f04fe972e9895d0f6ace2322ff44d0854287 Mon Sep 17 00:00:00 2001 From: Nathan Sculli <scull7@gmail.com> Date: Sun, 10 Jun 2018 18:51:34 -0700 Subject: [PATCH 2/2] Two more examples. Here are the exception problems in order of (imho) severity. * fromPromise (exception in callback) This is particulary insidious because it effectively squelches the error unless the user has the [unhandled-rejection] process handler defined. * mapOk (async exception) This causes problems because the exception is can't be caught by a traditional try/catch mechanism. Wrapping the call in a Promise may be a solution, though a bad one I think. * mapOk (sync exception) because of the synchronous context this exception may be caught, but the behavior is surprising, to me a least. [unhandled-rejection]: https://nodejs.org/api/process.html#process_event_unhandledrejection --- tests/TestFuture.re | 32 ++++++++++++++++++++++++++------ tests/TestFutureJs.re | 24 +++++++++++++++++++++++- 2 files changed, 49 insertions(+), 7 deletions(-) diff --git a/tests/TestFuture.re b/tests/TestFuture.re index 5de7dd2..1e1b246 100644 --- a/tests/TestFuture.re +++ b/tests/TestFuture.re @@ -4,11 +4,11 @@ exception TestError(string); type timeoutId; [@bs.val] [@bs.val] external setTimeout : ([@bs.uncurry] (unit => unit), int) => timeoutId = ""; -describe("Future", () => { +let delay = (ms, f) => Future.make(resolve => + setTimeout(() => f() |> resolve, ms) |> ignore +); - let delay = (ms, f) => Future.make(resolve => - setTimeout(() => f() |> resolve, ms) |> ignore - ); +describe("Future", () => { test("sync chaining", () => { Future.value("one") @@ -110,8 +110,8 @@ describe("Future Belt.Result", () => { | Error(e) => e |. equals("err2"); }); }); - - test("mapOk", () => { +/* + test("mapOk (sync exception)", () => { Belt.Result.Ok("ignored") |. Future.value |. Future.mapOk(_ => raise(TestError("boom, goes the dynamite!"))) @@ -121,6 +121,26 @@ describe("Future Belt.Result", () => { | Error(e) => raise(TestError(Js.String.make @@ e)) }) }); + */ +/* + 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)); + } + }) + }); + */ test("mapError", () => { Belt.Result.Ok("three") diff --git a/tests/TestFutureJs.re b/tests/TestFutureJs.re index fd2e2cd..c38e1ef 100644 --- a/tests/TestFutureJs.re +++ b/tests/TestFutureJs.re @@ -96,5 +96,27 @@ describe("FutureJs", () => { done_(); }); - }) + }); + + 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_(); + }); + }); });