Skip to content
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

RFC: what to do about promise rejection #16

Closed
aantron opened this issue Mar 22, 2018 · 23 comments
Closed

RFC: what to do about promise rejection #16

aantron opened this issue Mar 22, 2018 · 23 comments
Labels
Milestone

Comments

@aantron
Copy link
Owner

aantron commented Mar 22, 2018

Repromise is a proposed Reason binding to JavaScript promises.

Adding types to promises raises some new design questions, rejection being especially tricky. TL;DR: the current proposal is not to allow rejection in the main API at all.

I'm posting this issue to explain why that is, and to offer some notes about other approaches considered. Everything is open to experimentation and redesign, so please discuss :)

Feel free to skip around and read only some parts. The first two sections are important because they describe the problem and the main design considerations. Everything after that is a dump of attempted solutions and other thoughts.


The current main Repromise API looks like this:

type repromise('a);
  /* Only fulfillment type, 'a. */

let new_: unit => (promise('a), 'a => unit);
  /* Returns only a promise, plus function to fulfill it. */

let resolve: ...
let then_: ...
  /* No Repromise.reject, no Repromise.catch. */

There is a separate API, Repromise.Rejectable, that does have rejection, and provides catch for converting to "normal," non-rejectable repromises. This separate API is meant to be used mainly for writing the internals of bindings to JS libraries.






Background: JS interop

There are two parts to how Repromise interops with JS:

  1. How a repromise can be passed to JS.
  2. What to do with a promise coming from JS.
    • ...one that can't be rejected, and
    • ...one that can be rejected.

Passing repromises to JS is easy: every repromise is actually implemented as a JS promise, so you can declare bindings like

[@bs.val]
external takesPromise: Repromise.t(int) => string = "";

For receiving promises from JS, if the JS API provides a promise that the API never rejects, that promise can also be typed directly as a normal repromise:

[@bs.val]
external givesPromise: string => Repromise.t(int) = "";

The remaining, tricky case is when the JS API provides a promise that can be rejected. This is what Repromise.Rejectable is for:

module Repromise.Rejectable = {
  type rejectable('a, 'e);

  let new_: unit => (rejectable('a, 'e), 'a => unit, 'e => unit);
    /* This new_ returns a way to reject the promise. */

  let reject: 'e => rejectable(_, 'e);
  let catch:
    ('e => rejectable('a, 'e2), rejectable('a, 'e)) => rejectable('a, 'e2);

  let then_: ...
    /* The rest of the types differ from base API by having 'e parameter. */
};

Usage:

/* Externally, in the .rei file. */
let givesPromise: string => Repromise.t(result(int, char));


/* Internally, in the .re file. */
[@bs.val]
external givesRejectablePromise: string => Repromise.Rejectable.t(int, char);

let givesPromise = s =>
  givesRejectablePromise(s)
  |> Repromise.Rejectable.then_(i => Repromise.Rejectable.resolve(Ok(i)))
  |> Repromise.Rejectable.catch(c => Repromise.resolve(Error(c)));

Actually, the normal Repromise and Repromise.Rejectable have exactly the same implementation:

type repromise('a) = rejectable('a, never);

The types of all the functions, and that there is no way to construct never, mean that Repromise.Rejectable.catch must be called to convert rejectable repromises into normal ones. This is meant to encourage(/force? :p) bindings authors to offer any error-handling strategy other than native JS rejection: the example above suggests result. The reason for that is explained in the next section.

I am not sure how useful Repromise.Rejectable would actually be in practice. For example, if a JS API rejects a promise with two different types, then you have to write custom JS code to bind it. A special, and perhaps common, case of this is a JS API that explicitly rejects promises with some type like int, but also raises exceptions that get converted into rejections by JS.


Problem: Difficulties typing rejection

Briefly consider fulfillment instead of rejection. In JS, any promise can be fulfilled with a value of any type. You can race an array of promises for ints and strings, and the resulting promise can be fulfilled with either int or string.

In Reason, however, the "obvious" promise API looks like this:

type promise('a);
let race: list(promise('a)) => promise('a);

...and that constrains you to racing only promises that can be fulfilled with values of the same, one type. Typing benefits the safety of the API elsewhere, but it makes race more restrictive, to an extent not directly related to safety.

Nonetheless, the restriction is still pretty reasonable.


Back to rejection: in JS, any promise can also be rejected with a value of any type. The most "obvious" encoding of this is probably what's done in Repromise.Rejectable:

type promise('a, 'e);
let race: list(promise('a, 'e)) => promise('a, 'e);

...except now, you can only race promises that are both fulfilled and rejected with values of the same respective, single types. In practice, this seems way too restrictive and annoying. Rejection is typically a bit of a background mechanism or an afterthought, so it is very bad when interference from typing rejection prevents you from writing a reasonable program.

A problem arises even with then:

type promise('a, 'e);
let then_: ('a => promise('b, 'e), promise('a, 'e)) => promise('b, 'e);

/* usage: */
... promise |> then_ callback ...

Now, the first promise, representing a "first" asynchronous operation, and the second promise, returned by the callback, have to be rejectable with the same type.

This means that actually taking advantage of the fact that 'e is a type parameter, and using different types for it, will probably make most code too difficult to refactor or write. For the code to be composable, it will often have to use both then_ and catch right next to each other, in order to be able to vary both 'a and 'e from one promise to the next.


Attempt 1: Unityped rejection

This suggests a solution: make one good choice of a type for 'e, and always use that. This is actually pretty common:

  • JS has unityped rejection: with its only type of "any JS value."
  • Lwt has unityped rejection: with exn (native OCaml exceptions).
  • The previous point suggests rejection with any other single open variant type (type rejected = ..;). I chose not to commit to this because you can't have an exhaustive pattern-matching on an open variant type, and because it's not directly compatible with rejections coming from JS anyway: JS will never generate OCaml open variant values.

There are two "degenerate" choices of one type for rejection:

  • Rejection with "no payload"/unit/Top: if we want to propagate rejections using the native JS rejection mechanism, but don't want to bother accurately typing it and don't want to allow values to be carried around by it.
  • No rejection/Bottom: this is what Repromise currently has. There is no way to construct never, so there is no way to trigger rejection of a normal repromise.

Attempt 2: Polymorphic variants

Polymorphic variants have the advantage that the compiler can often simply infer the correct rejection variands. However, almost every user of Repromise would likely have to deal with reading and writing either complex type annotations

let mySimpleFunction: int => Repromise.t(char, [> `String of string]);

or casts

takesPromise((p :> Repromise.t(char, [`String of string])));

In fact, in many places, one is forced to choose one or the other. This seems like a major drawback for a general-use library.

I also considered polymorphic variants for a phantom type to express the distinction between rejectable and non-rejectable promises:

type non_rejectable = [ `Non_rejectable ];
type rejectable('e) = [ non_rejectable | `Rejectable('e) ];

let new_: unit => (promise('a, [> non_rejectable]), ...);
let newRejectable: unit => (promise('a, rejectable('e)), ...);

let then_:
  ('a => promise('b, [> non_rejectable] as 'r), promise('a, 'r)) =>
    promise(b, 'r);

...but it's not clear what is achieved at the cost of such complexity, compared to two separate APIs as proposed now.

One of the problems with such designs is that they make everyone aware of rejection typing. By constrast, without rejection in the main API, hopefully users that stay in the "Reason world" have much less to learn. Only bindings authors need to deal with more complex types in some cases. Even for bindings authors, however, complex types are not very useful. Bindings authors are working with FFI, so they already have the opportunity to write some code in JS and/or assign arbitrary, but straightforward, types.


More background: What is rejection?

This is probably easiest answered by asking "how is rejection different from fulfillment?" Both resolve a promise, but

  1. Rejection doesn't trigger the callback call in then.
  2. Rejection causes early resolution of all.
  3. Exceptions raised during a callback are converted into rejections.

(1) is properly the job of the option and result types:

type option('a) = None | Some('a);

let mapOption = (callback, option) =>
  switch (option) {
  | None => None
  | Some(x) => Some(callback(x))
  };

We can provide helpers for mixing Repromise with option and result to model this kind of error handling.


(2) seems like just a design decision. Not using native rejection means that if we want early rejection of all, we would have to write our own implementation of all for option and/or result types.


For (3), Repromise currently forwards all uncaught exceptions in callbacks to a function

let onUnhandledException: ref(exn => never);

which kills the program by default. We should probably change this behavior to just printing the stack trace. A promise that would have been rejected by a failing callback in JS is left pending by Repromise.

We can provide a function like

let tryWith: ('a => repromise('b)) => ('a => repromise(result('b, exn)));

for catching exceptions in callbacks and converting them to explicit error handling. We could also extend onUnhandledException into something close to Async monitors.

I'm not yet sure what types to use for exception handling: exn? Js.Exn.t? And, trying to type catching JS exceptions, unwrapping the carried values (of arbitrary type), and using that to reject promises seems like it would create the same unfavorable composability situation as described in the "problem" section.

Dealing with exceptions explicitly, rather than converting them to rejection and propagating rejections around, could be a bit burdensome. But, I think the set of programs people write in Reason can be roughly split into two, and exceptions as rejections might not be that desirable or necessary in either case:

  • High-reliability programming typically wants explicit error handling.
  • In short scripts, etc., the programmer wants to be told about the problem to debug it, but is less concerned exactly how the error is propagated otherwise. Rejection is not necessary here, it's enough to show a good message and/or stack trace.
@ncthbrt
Copy link
Contributor

ncthbrt commented Mar 22, 2018

Suggestion 1

Have a specialisation of repromise which allows ergonomic treatment of a result type. This will mitigate the downsides of eliminating rejection from the core construct. This would basically be an eager maybe monad.

Suggestion 2

A single function which can handle both resolutions and rejections to handle exceptions in the termination of the pipeline:
let subscribe: (~onResult: 'a => 'b =?, ~onError: exn => 'c=?, repromise('a)) => unit;

This is analogous to how observables work

@chenglou
Copy link

Cc @jaredly

@jaredly
Copy link

jaredly commented Mar 22, 2018

@ncthbrt
Suggestion 1: 👍
Suggestion 2: Wouldn't you want the onResult and onError to both return unit?

@aantron
I'm totally on board this method. I've thought a bit about this problem as well, and this is pretty much where I landed -- just have rejectionless promises for things that stay within reason-land.

@ncthbrt
Copy link
Contributor

ncthbrt commented Mar 23, 2018

@jaredly I was originally thinking that. But often a side effect will return a value instead of unit. Having the subscribe function not caring about the return type means you don't have to have ignores everywhere.

@aantron
Copy link
Owner Author

aantron commented Mar 24, 2018

@nthcbrt Yes to suggestion 1. I just hope it doesn't bloat the size of the implementation, but I suspect it's going to be ok (current compiled repromise.js is about 1K gzipped).

I'm not sure yet how suggestion 2 would play out. Would we make the rejection type always exn, and use that to implicitly convert exceptions into rejections (as done in JS)?

@ncthbrt
Copy link
Contributor

ncthbrt commented Mar 24, 2018

@aantron: Yes making the rejection type exn would reinforce that it is for things which are actually exceptional. If this were to happen, thinking about it more, it is imperative that any sort of rejection handler return unit. This discourages it being used an alternative means of control flow.

@aantron
Copy link
Owner Author

aantron commented Mar 24, 2018

So, if I understand correctly: on a type level, the API would still not look like it supports rejection for the most part. Exceptions would be (a bit quietly) turned into JS rejections, and could be extracted later using subscribe.

@jaredly
Copy link

jaredly commented Mar 25, 2018

@ncthbrt I would much rather require more usage of ignore than to lose values that need to be cared about. If you have a side-effectful function returning a value (like a success boolean etc.) you should be mindful of it

@ncthbrt
Copy link
Contributor

ncthbrt commented Mar 25, 2018

@jaredly Fair point.

@phated
Copy link

phated commented Mar 28, 2018

After dealing with Js.Promise and Js.Exn from BuckleScript, I really don't want to deal with rejectable promises in my Reason code.

That being said - When I finally got my head wrapped around promises in JS, I've always viewed them as JS's version of Result('a, 'e) (async being consequential) so when I see Repromise.t(result('a, 'e)) my brain is resolving that as Ok(Ok('a)) or Error(Error('e)).

Is there a way that the Repromise.t could be represented as the result so it doesn't feel like a double-wrapped result?

@johnhaley81
Copy link
Contributor

Totally agree. I think the ideal type signature would be:

Repromise.t([> `Error('e) | `Ok('a) ])

No rejection madness, but still have a way when wrapping a JS lib's promise to catch exceptions (a.la https://github.com/wokalski/vow) and then at the end you would get the

`Error('e)

in reason land.

@aantron
Copy link
Owner Author

aantron commented Mar 30, 2018

@phated What do you mean by represented? I'm assuming not implementation details, but something visible from the user's point of view?

I can see how Repromise.t(result('a, 'e)) can produce the impression of double-wrapping. Indeed, that impression is accurate :) The two cases that type is using are Ok(Ok('a)) and Ok(Error('a)). My hope is that people eventually "forget" about promise rejection in Reason-land, so the outer Ok (the one baked into JS promises) becomes gradually "forgotten," too, when working in Reason.

@johnhaley81 People would be free to use that type signature with Repromise.t. We should probably eventually "bless" one method of error handling, by supporting it in a helper module of Repromise. That may be Repromise.t(result('a, 'e)), or Repromise.t([> Ok('a) | Error('e) ]), or even something completely different. I think we will probably need a period of user experimentation before doing it.

@danny-andrews
Copy link

@ncthbrt

A single function which can handle both resolutions and rejections to handle exceptions in the termination of the pipeline:
let subscribe: (~onResult: 'a => 'b =?, ~onError: exn => 'c=?, repromise('a)) => unit;

I would prefer to see onError come before onResult. It forces you to deal with the error case. Or is this a tuple? I'm not familiar with the type notation.

@aantron
Copy link
Owner Author

aantron commented Jun 26, 2018

@danny-andrews These are separate labeled arguments, not a tuple, which is as you seem to correctly suspect :)

@shinzui
Copy link

shinzui commented Dec 2, 2018

Inspired by Jane Street's Error module, I standardized on ('a, Error.t) Result.t Repromise.t, and I a added a bind function to make it easy to compose. That's been working really well.

@aantron
Copy link
Owner Author

aantron commented Dec 2, 2018

Thanks!

@danny-andrews
Copy link

Just wanted to share a story from the Scala community about moving from a design similar to the one suggested here to a bifunctor design.

http://degoes.net/articles/effects-without-transformers

tl;dr: working with nested monads can be unruly. Monad transformers make it nicer, but have significant performance considerations.

Not sure how applicable it is to ReasonML (I don't know enough about to runtime to say), but thought it might be interesting reading in the problem domain nonetheless.

@danny-andrews
Copy link

A few people in this thread have expressed frustrations with dealing with Promise rejections in JS.

@phated

I really don't want to deal with rejectable promises in my Reason code.
@johnhaley81
No rejection madness

Can you elaborate on what you mean by this? I personally hate the fact that errors can be swallowed simply by forgetting a catch. But an implementation in Reason wouldn't have to suffer that problem, simply by making the error handler the first parameter when forking a promise, similar to how it works in flutures.

It seems like 90% of use-cases for promises will have failure conditions (most async actions are I/O and can fail in some way) so it seems to make sense to encode that into the construct in some way.

@phated
Copy link

phated commented Dec 4, 2018

@danny-andrews a promise doesn't need to reject to force you to handle errors. https://github.com/RationalJS/future and https://github.com/wokalski/vow handle this by making the interface Result-ish (the sam patterns can be achieved with Repromise but it doesn't have the utility methods).

My ideal interface (and the one I pretty much described above) is actually the same as Vow.Result but that library feels incomplete and not really maintained.

@lessp
Copy link

lessp commented Aug 6, 2019

@danny-andrews a promise doesn't need to reject to force you to handle errors. https://github.com/RationalJS/future and https://github.com/wokalski/vow handle this by making the interface Result-ish (the sam patterns can be achieved with Repromise but it doesn't have the utility methods).

My ideal interface (and the one I pretty much described above) is actually the same as Vow.Result but that library feels incomplete and not really maintained.

hey @phated, by 'Result-ish', do you mean result-helpers as mapOk, mapError etc? I've been meaning to ask what the stance is on these kind of helpers. 🙂

fetch("...") /* Repromise.t(result('a, 'e)) */
|> Repromise.mapOk(({body, _}) => Ok(body))
|> Repromise.wait(log)
|> Repromise.mapError(...)

I'm currently experimenting a bit with Repromise on the native-side and would like errors to "return early" and skip the success-pipe. If that makes sense.

@aantron
Copy link
Owner Author

aantron commented Aug 7, 2019

We should add Result helpers.

@aantron aantron added this to the 0.6.2 milestone Aug 16, 2019
@aantron
Copy link
Owner Author

aantron commented Sep 10, 2019

Ok, I just added Result and Option helpers, sorry for the burnout!

They look like this:

/* Results. */
let andThenOk:
  ('a => promise(result('b, 'e)), promise(result('a, 'e))) =>
    promise(result('b, 'e));

let andThenError:
  ('e => promise(result('a, 'e2)), promise(result('a, 'e))) =>
    promise(result('a, 'e2));

let mapOk:
  ('a => 'b, promise(result('a, 'e))) => promise(result('b, 'e));

let mapError:
  ('e => 'e2, promise(result('a, 'e))) => promise(result('a, 'e2));

let waitOk:
  ('a => unit, promise(result('a, _))) => unit;

let waitError:
  ('e => unit, promise(result(_, 'e))) => unit;



/* Options. */
let andThenSome:
  ('a => promise(option('b)), promise(option('a))) => promise(option('b));

let mapSome:
  ('a => 'b, promise(option('a))) => promise(option('b));

let waitSome:
  ('a => unit, promise(option('a))) => unit;

I think with the way I coded the result helpers, we need BuckleScript 6.x (with OCaml 4.06 support), but if this is a problem, we can probably easily relax that to 4.02.

Next, I'll add some tests next, and get rid of the dep on Curry (again) to reduce the bundle size to 3K or so (again).

I'll maybe also alias andThen as flatMap, and wait as tap, and all the corresponding Result and Option helpers.

@aantron
Copy link
Owner Author

aantron commented Sep 10, 2019

The bundle size is now ~4K. I can get it down to 3.5K by deleting support for Option. On reflection, Option is probably much less useful than Result, because async errors will almost always carry a payload. However, I don't know if losing Option from the API is really worth 500 bytes of savings.

The reason Option is 500 bytes is because BuckleScript has a fancy representation for options, trying gto map None onto undefined. As a result, there are some non-trivial helpers that need to be pulled in if your code uses options at all.

I imagine most Reason projects are using Option anyway, though, and will have the dep no matter what.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

9 participants