Async Error Handler Running Synchronously

[alexlafroscia]

I'm running into a pretty bizarre error while trying to upgrade our app to ember-concurrency@2.0.2 (from the latest pre-2.0.0 release).

It seems that the async error handler defined here is in fact running synchronously, and prempting our own error handling.

We have a swallow-error helper that we sometimes use for cases where an Ember Concurrency task would expect an error to be thrown and we use the derived error state on the task instance to show it to the user. Because we error will be "handled" in the UI itself, we want to swallow the error entirely.

swallow-error helper source

import { helper } from '@ember/component/helper';

/**
 * This helper is meant to be wrapped around another action that might throw an error that we want to suppress.
 *
 *   ```hbs
 *   <button {{on 'click' (swallow-error (perform this.someTaskThatMightThrow))}}>
 *     Click Me
 *   </button>
 *   ```
 */
export function swallowError([fn]) {
  return function callAndSwallowError(...args) {
    try {
      const response = fn(...args);

      if (response.catch) {
        return response.catch(function () {
          // Swallow async error
        });
      }

      return response;
    } catch (e) {
      // Swallow synchronous error
    }
  };
}

export default helper(swallowError);

In the current version of ember-concurrency that we're using, this works just fine.

In ember-concurrency@2.0.2, it seems like the code here that should report the error if it was not already caught is being executed synchronously when it should be executed asynchronously

ember-concurrency/addon/-private/external/task-instance/executor.js

Lines 344 to 348 in 35a188c

	this.env.async(() => {
	if (!this.asyncErrorsHandled) {
	this.env.reportUncaughtRejection(this.state.error);
	}
	});

I know it's a challenge to grok a stack trace without much context, but this is what I'm seeing; a single synchronous trace from my debugger point inside the error-reporting callback that should be run asynchronously straight down to the function invocation inside callAndSwallowError.

I know that env.async is using join from the @ember/runloop package, which will insert the callback into the current queue. My guess is that Backburner is calling things in the current queue before proceeding with execution of the code in my helper... for some reason. My code isn't doing anything directly with the runloop; there's just the normal amount of wrapping that helper invocations have by default at play.

I tried to create a failing test case in the ember-concurrency repository but have been unable to get the same error to occur there just yet. I will post a PR with that once I have it.

I'm wondering whether a potential fix could be specifically scheduling the async callback into the next run loop, rather than using join which can insert it into the current "tick".

[alexlafroscia]

Comparing stack traces between my app (with the problem) and the ember-concurrency tests (without the problem) is that in my tests, at the point that the task is performed (while wrapped in my promise-swallowing helper) the run-loop is empty, and thus a new one is created, while in the ember-concurrency test the runloop is already populated.

In terms of the actual methods being queued, in this code:

https://github.com/BackburnerJS/backburner.js/blob/6a720fcf1286b500e7bbf892bc25a055c779696c/lib/index.ts#L622-L624

The Ember Concurrency async error-handler is run synchronously if currentInstance is null; if not, the async error handler does what you would expect!

[maxfierke]

Mind sharing some code from the task too? Mostly curious about whether the error is happening before or after the first yield.

[alexlafroscia]

Before -- this is the task definition

  @task
  *requestCertificate() {
    if (!this.customDomainCertificate.domain) {
      const error = new Error();
      error.type = 'domain';
      throw error;
    }

    yield this.customDomainCertificate.save();

    yield this.customDomainCertificate.request();

    yield this.customDomainCertificate.fetchValidationRecords();
  }

[alexlafroscia]

Interestingly, I updated ember-qunit to the latest version in the ember-concurrency repo, and sure enough, the test I wrote in the ember-concurrency test suite is now also failing! So in some way, ember-qunit influences the state of Backburner at the point that the code is running, which influences whether the "async error handler" is actually run asynchronously or not!

[alexlafroscia]

Maybe, to ensure that the async error handler is actually called asynchronously, we could use later from @ember/runloop instead, with a timeout of 0 or 1? I'm not sure if 0 queues for the next "tick" in the same way as setTimeout, but I would assume it does.

[maxfierke]

@alexlafroscia can you share the test case here or in a PR?

Using later with 1 or next might work, but I'll need to do some thinking about what makes sense here. later with 1 is already used by reportUncaughtRejection, so I wonder if that might effectively make it two "ticks"?

I think we roughly used run in 1.x (with setTimeout if there was no runloop... :/) so maybe there's some subtlety of nested runloops that would help here, but I'd have to play around with it.

[alexlafroscia]

@maxfierke very-much-messy PR opened with the test I wrote into the test suite that fails

[maxfierke]

having a bit of difficulty reducing this, but curious what happens if you do const response = await fn(...args); instead (then you probably don't need the .catch check and call, as it'll all be handled by the normal try/catch semantics)

Tried reproducing it outside of a helper, and I can't :/

[maxfierke]

Alrighty, I think I'm starting to get a handle on this. To be clear: the error reporting is happening asynchronously, but there's an ordering issue in which the uncaught error reporting is running before the catch chain has been called on the TaskInstance, so the internal flag for async error handling hasn't been set to false by the time the error reporting code runs. I'm not 100% sure why the delay. Maybe has to do with how helpers are evaluated or something (or the perform helper in particular)

Seemed to be able to fix the failing tests by wrapping the call in Promise.resolve().then(...) to push it to the next tick while within the runloop, so that the catch has a chance to register itself before the throw happens. Test suite passes, but need to think more about whether this is more correct

[maxfierke]

Alright, root cause here seems to be due to the run.bind wrapping of the perform helper. Basically, it's wrapping perform in a full runloop, which means that a synchronous task throw will be reported within that same invocation, which is before the (swallow-errors) helper is able to come in an instrument the task instance. Since it's already run, it's too late. I don't think we need this runloop binding anymore, since there's no auto-run assertion with the versions supported by ember-concurrency 2.x, so I will look into whether it's a breaking change to simply remove it. FWIW, the test suite likes it fine, but I want to run it against a bigger, more complex app in order to ensure it's not creating additional problems. Will post a PR once I verify it doesn't cause any issues.

[alexlafroscia]

Sounds good! Thanks for getting to the bottom of this!!

[alexlafroscia]

Thanks again @maxfierke for putting so much effort into this admittedly-obscure bug!