Synchronous exceptions thrown from complex expressions create abandoned promises. Solutions?

💡 Ideas
21

This, I believe, is transactional pattern: you generally have to provide an ability to commit or roll back from such an incomplete state. It also can be conveniently wrapped in a higher-order functional bracket, although in that case you have to make additional provisions for a rollback mechanism.

Yes, I do think this was an unfortunate design decision, but it was a compromise in favor of a simpler API. I generally add a form of catch-log-throw wrapper around asynchronous operations passed into Promise.all() to avoid silent errors.

22

There's that of course. C++ at least had explicit destructors -- with a multitude of rules associated with their order of execution during stack unwinding, some paths leading to an immediate abort :-( -- GC-based languages that completely forgo object lifetime tracking in favor of an explicit finally suffer more from these issues. Still, adding asynchronous expressions to language as a first-class construct calls for close scrutiny of edge cases IMO -- otherwise language constructs that look natural and tempting sneakily drag in fundamentally broken edge cases.

23

Yes, this is very similar indeed! It's just that I would probably write that code as:

let value2;
const [value1, value3] = await Promise.all([
  getValue1(),
  getValue3(value2 = await getValue2())
]);
return {value1, value2, value3}

(assuming the use of value4 in your example was a typo), but it would lead to the same outcome: await-subexpression is essentially treated as synchronous by the rest of the expression.

Bad news, bad news all around :-)

--- I am limited in my number of posts, so adding the response here :-( ---

Forget the keyword for now, because the wrapper for lazy values does the same job. I think:

let value2;
const [value1, value3] = await Promise.all([
  getValue1(),
  promise( async () => getValue3(value2 = await getValue2()) )
]);
return {value1, value2, value3}

It is kinda awkward in that you actually have to add async to the thunk and the wrapper is not doing any useful work at all in this case except evaluating the lazy value -- the thunk passed in will catch its own exception from the await and convert it to a promise -- but at least it still works the intended way. A language feature could of course optimize the extra promise away.

24

Yep, that was a type-o - fixed.

25

Well, I'm not sure what the best solution to this problem is, but whatever the solution is, it should probably be flexible enough to provide fixes for both of our code snippets. (Even with how you rewrote it, it'll still suffer from the same problem, and I don't think the new promise keyword would help it).

Though referring to what @ljharb said, if ECMA thinks of rejected promises as a no-op, and node is not following ECMA's recommendation here, then this issue is a node-only issue and not an issue with the language itself. So, it seems like the solution will fall into one of these categories:

  • Convince TC39 to change their stance on rejected promises and follow in node's footsteps. Such a proposal could include extra syntax (like your proposed "promise" keyword) to make such a change easier.
  • Convince node to change their stance and follow in ECMA's footsteps. Maybe find other kinds of proposals to make working with rejected promises easier, without making the language treat rejected promises as fatal by default.
  • Ask node to provide better support for their decision. They chose to break away from what ECMA intended, and as such, they should probably be the ones responsible to provide the resources needed to make their diversion not so awkward to deal with. Maybe they can provide some built-in functions to help out.

The first two options seem highly unlikely to happen, so our best bet might be to move the conversation over to the node team, and figure out what can be done with what javascript provides to make their decision easier to work with.

If you do choose to do this, I would love to watch that conversation progress, as it would be nice to have a good standard solution to these issues in node.

26

@ljharb What "language design" are you referring to? Sure a rejected promise is not fatal, just like a thrown exception isn't. The problem are unhandled rejections or exceptions - and I don't see why those shouldn't crash the process. Sure, handling errors in concurrent code is harder than in sequential code, but the outcome shouldn't be any different.
Also could you please clarify whether this is your personal opinion or an official stance of the TC39?

1 Like
27

Ah, that's a though one. I would recommend to write it as

function fetchUserData(params) {
  try {
    const promise2 = getValue2(params)
    const [value1, value2, value3] = await Promise.all([
      getValue1(),
      promise2,
      promise2.then(getValue3),
    ])
    return { value1, value2, value3 }
  } catch (err) {
    if (err.type === NOT_LOGGED_IN) return null
    throw err
  }
}

or, if a binding for value2 in the getValue3 computation is preferred,

function fetchUserData(params) {
  try {
    const [value1, values2and3] = await Promise.all([
      getValue1(),
      (async () => {
        const value2 = await getValue2(params)
        const value3 = await getValue3(value2)
        return { value2, value3 }
      })(),
    ])
    return { value1, ...values2and3 }
  } catch (err) {
    if (err.type === NOT_LOGGED_IN) return null
    throw err
  }
}

The IIFE might be extracted into a named helper function to further signify the separate computation that's running concurrent with getValue1.

28

I never thought of your first code snippet, that's smart!

I had ended up reworking it to be something similar to your second example.

29

Promises were introduced in ES2015, and engines had no capability to intercede with an unhandled rejection at that time.

The capability was added here: Add HostPromiseRejectionTracker by domenic · Pull Request #76 · tc39/ecma262 · GitHub as part of ES2016, and the entire purpose of that was to enable browsers to provide useful console logs when an unhandled rejection occurred (and, explicitly, not to halt execution of anything).

node chose to, imo, abuse this slot to do more than the intended logging, when they defaulted to terminating the process. Note "non-observable change" in the PR - node makes it observable.

If there was a way to specify "execution must continue after this point", then node's choice would be forbidden by spec, but since implementations have all sorts of reasons they might need to terminate at any point, such a requirement would not be even remotely tenable, despite matching the spirit of unhandled rejection hooks.

30

Well, that's exactly what browsers do on an uncaught exception inside an event handler. "Halting execution" in a browser doesn't really have any semantic meaning, short of oopsing the entire page because the event loop is logically associated with the page rather than an OS process.

Node, on the other hand, chose to treat an unhandled rejection the same way it treats uncaught exceptions that propagate to the level of the event loop itself: break the event loop and terminate. This behavior is both internally consistent and consistent with most other service architectures. It may not play well with personal preferences; it also introduces an inconsistency to the Javascript code intended for dual use, but it is certainly not unreasonable.

2 Likes
31

I've gotten unhandled rejection errors in that exact kind of scenario in the wild with Chrome while writing a userscript and had to wrap it accordingly. I feel a spec note might be necessary to clarify that the promises subscribed to in Promise.all should remain considered handled even in the face of collection of the state that "handles" it, along with maybe a Test262 test (assuming it's possible to write) to verify they don't complain about it. But yeah, this is not worth a new proposal. The spec isn't broken - implementations (or at least V8) are.

Edit: cc @ljharb

32

To me this seems like an API problem. What is being called that throws synchronously but returns a promise? If the return type is promise then errors should be returned as rejected promises and not synchronously thrown. async functions behave this way. Web APIs like fetch() behave this way. Other functions should too. If they don't, then wrappers like promise() would be necessary to account for this behavior.

33

The first error is propagated through as a rejection. The other rejections should be handled, but instead, V8 at least is reporting it as an unhandled rejection when it's very much not. It's an implementation bug that TC39 could stand to do a better job at making their intent clearer on.

There's nothing about synchronous exceptions going on, and only tangentially does this deal with exceptions at all. This is all a misinformed GC.

34

My understanding was that this was specifically about synchronous exceptions, though it is possible I missed an additional detail in the rest of the thread.

Here, the bar rejection is considered unhandled because all() never gets a chance to be called due to the throw. If baz didn't throw, and instead rejected, then the all would get called handling both.

35

Oh, okay. Different than what I experienced though with pure promises (happens with the idiom Promise.all(list.map(async item => ...))).

36

I think this post explained the nature of the use case better: Synchronous exceptions thrown from complex expressions create abandoned promises. Solutions? - #13 by MaxMotovilov

37

Same problem, though we're changing where we're pointing the finger. The caller should understand their approach is effectively:

rejected = getRejectPromise()
throw Error('bye')
rejected.catch(handled)

While the above is a little more obvious than in the all() case, it boils down to being the same thing. If what's handling a promise (whether it be all() or catch()) can't run after the creation of that promise, the promise will go unhandled if rejected.

The consequences of unhandled rejections are a different issue. We're kind of in a special case here because in calling all(), your intent is to allow rejected promises beyond the first to effectively be a noop. But if all() never gets to run in the first place, how is the runtime supposed to know that?

1 Like
38

It can't, not without significantly complicating the semantic model of JS expressions. That was sort of my point -- I'm just looking for the least verbose, and most "FP" if you will, solution possible.

39

@MaxMotovilov You mentioned earlier that when you figured out about this issue, you felt that "Javascript is irretrievably broken". Just out of curiosity, if you had the power to redesign things from the ground up, how might you have done it differently to avoid this problem?

I'm just feeling like what @senocular said is correct, in that all of these examples we've been bringing up are just different forms of creating promises, then having an error get thrown before we set a promise handler - and I'm at a loss as to how this could have been designed differently. Maybe, if I had a picture of a general-purpose, theoretical solution in my mind that could solve this issue, then we could find a way to adapt it to work in real javascript.

Because, again, I agree these are real issues we're bringing up. I just can't think of any good solution to this problem, and maybe that's because there really isn't one. Maybe the right solution is simply that the caller has to be careful to make sure they always bind some handler onto their promises before letting other code get executed.

40

Yeah, because such a trivial thing as the order of sub-expression evaluation may, completely opaquely, cause a crash-level error. To add insult to injury, it happens exactly in conjunction with the one library facility supposed to provide concurrent evaluation of asynchronous expressions... and what's the raison d'etre of async if not concurrent evaluation?

Come to think of it -- and I am just realizing this as I type, hah -- perhaps if concurrent await was a first-class construct in the language, it could have been handled better?

Let's assume we extend await to optionally support a list of concurrent asynchronous expressions. Note that this is emphatically not the same as "accept a list of promises", because it associates semantic meaning with the context of "argument-of-await" as will be demonstrated. Let's say we have a special syntax, "await[ list-of-expressions ]" in addition to the plain "await expression". Now we say that an expression that is an argument-of-await is evaluated as follows:

  • If it terminates normally, evaluating to a promise, that promise is its value
  • If it terminates normally, evaluating to a non-promise value, its value is a promise resolved to the value it evaluated to
  • If it terminates abruptly, its value is a promise rejected with the exception thrown, or with the exception synthesized by runtime if the nature of abrupt termination is different from a thrown exception (I'm not sure if that's actually possible but the abrupt termination language in the standard includes other alternatives like break/continue and I don't want to spend time digging into edge cases like Duff's device right now)

So we've just covered the use case I was in such a tiff about: synchronous exception from an asynchronous-valued expression will not create orphaned promises assuming it is evaluated in the context of argument-of-await because it will be converted to a promise at the boundary of that context -- either the closing bracket or a comma separating arguments. Of course we now effectively have two commas with different behavior, "sync comma" in normal expressions and argument lists and "async comma" at top level inside "await[ ... ]". But that feels like a reasonable price to pay for having a separate syntax for async expressions.

Now I think we can get extra mileage from the new syntax by considering what is the value returned by await. In case of the b/w compatible, single argument await, it would still be the value the promise has resolved to, which is only reasonable -- there are no obvious alternatives to the current behavior. In case of the new await[...], we are free to evaluate to something other than an array of promise values.
There still needs to be a way to [synchronously] get the values from outside the [resolved] await and to propagate exceptions, but it may well be done explicitly. Assuming that await[...] returns a library object with multiple available methods, we could have

const listOfValues = await[ foo(), bar() ].all();

or

const oneValue = await[ foo(), bar() ].race();

or a more elaborate API that can implement custom strategies such as race-to-the-first-but-preserve-the-rest or collect-all-not-failed or what have you. If cancellation "backchannel" was added to promises, it could also make use of that as optimization.

I guess I rambled enough... it does look like language improvements are not only possible but also feasible, although they aren't by any means obvious.