Should wrapped functions send their errors to HostReportErrors (i.e. window.onerror)?

[domenic]

@mhevery In my initial spec for Zone.prototype.wrap I've just had it act similarly to calling z.run(cb). Errors are rethrown.

In previous discussions you thought it was necessary to use "runGuarded" functionality here, where errors are not rethrown but instead sent to window.onerror (or, in the future, a zone-specific error handler).

However, I'm no longer convinced this is the right design. I think we can accomplish everything we want with errors being rethrown. We just have to make sure that callers like EventTarget continue to send thrown errors to window.onerror (and, in the future, the relevant zone).

Can you remind me why you thought it was necessary to catch the errors and send them to window.onerror whenever wrap is used?

[mhevery]

1. Think of a library as a black box, where you hand it a callback and it calls you when it sees fit.
2. In such a case we can separate it into two kinds of libraries
   • (a) Libraries such as Promise and Observables where throwing an error has actual meaning to those libraries (This is the minority)
   • (b) Libraries where they can't do anything useful with the error. (This is a majority of libraries). So throwing an error into the library makes no sense and Zone should process it.

Now most browser APIs fall into 2(b) category. For example div.addEventListener('click', () => { throw "Error"; }). There is nothing of value that addEventListener can do here. It just so happens that the addEventListener forwards the error to the global error handler. So while all browser APIs forward to global handler because they are the last frame on stack, most libraries do not handle errors, and could get into inconsistent state.

So the safe thing to do is to say that when we are passing a callback into the library:

1. we simply call wrap and forward the errors to the global error handler. (Fail early)
2. If the library is of the small subset which do care about the errors, then those libraries need to manage the zones explicitly, using run.

[domenic]

I don't think that is the correct conclusion from your premises. 2(b) libraries like EventTarget can do something useful with the error: they forward it to the global event handler. But that's a very specific decision that EventTarget made. Other libraries (e.g. Node's EventEmitter) just let the error bubble (so ee.emit("x") with a throwing handler will itself throw). This allows callers to recover as they see fit, either by letting it bubble to top-level anyway, or by using try/catch at the call site.

To me, the conclusion is that we should instead add a primitive to the platform like the following:

window.runGuarded = function (cb) {
  try {
    cb();
  } catch (e) {
    window.dispatchEvent(new ErrorEvent({ message: e.message, filename: parseFilename(e), lineno: parselineNo(e), colno: parsecolNo(e), error: e }));
    // Or, in the future: call Zone.current's error handler with e.
  }
};

Then libraries who want to censor thrown errors can easily do window.runGuarded(f), whereas libraries which want to allow their callers to catch errors can just call f().

I don't think, however, that the behavior of zone.wrap should automatically use this window.runGuarded function.

[mhevery]

The event handling must be done inside the wrap method, because by the time the exception gets to the underlaying library, the library no longer has access to the zone from which the exception originated, and hence does not have the correct handler for the exception.

[domenic]

How would doing the exception handling in the wrap method help with that at all?

[domenic]

OK, I think I see...

addEventListener(name, cb) {
  this._eventListeners.append(name, Zone.current.wrap(cb));
}

dispatchEvent(name, e) {
  for (const listener of this._eventListeners.get(name)) {
    try {
      listener(e);
    } catch (err) {
      // we want to send err to listener's zone, not to Zone.current. Problem.
    }
  }
}

There are a lot of potential solutions here besides having wrap implicitly do runGuarded behavior, though. Let's brainstorm a bit.

1. Add the ability to get a wrapped function's zone. Then we could do Zone.getFunctionZone(listener) and call that Zone's error handler.
2. Make window.runGuarded automatically do that desired logic based on its argument's zone. Then we could replace the try/catch with window.runGuarded(listener, undefined, e). (Note: just window.runGuarded(() => listener(e)) doesn't work well since the arrow function's zone is not correct.) This is probably better named Zone.callGuarded at this point.
3. Implement addEventListener not to store the wrapped function, but to store { zone: Zone.current, cb }. Then we could call listener.cb(e) and inside the catch do listener.zone.handleError.

I'm not sure if these are better or worse than having wrap run things with special behavior. They're certainly more low-level and flexible, which is a plus. (2) seems pretty good, in particular.

[mhevery]

1. would put too much responsibility on the caller/library. Way to easy to forget and get it wrong.
2. window.runGuarded can not do it because by the time the exception gets to it it has passed through the wrap closure and the zone has been restored, hence lost.
3. Sure this would work in the same way that this is what Promises does. But again, I would say that it places too much responsibility on the library to get this right. Yes browsers can get this right, but I would not expect third-party libraries to get this right.

I think wrap should just catch and delegate to the error handler. There really is not much reason to pass the error onto the library.

Could you explain your motivation why you don't want to catch errors? What is the issue you are concerned with?

[domenic]

Please read (2) again. You can make window.runGuarded/Zone.callGuarded send the exception to the right zone.

I'll respond more later today.

[mhevery]

Sorry, not following, Could you show actual code implementation of how that would work.

[domenic]

Sure.

Zone.runGuarded = function (f, thisArg, ...args) {
  const zoneForF = GetZoneForWrappedFunction(f);
  try {
    zoneForF.run(() => f.apply(thisArg, args));
  } catch (e) {
    zoneForF.handleError(e);
  }
};

(In reality you wouldn't do the enter-run-exit-reenter-handleError-exit dance but instead just enter-run-handleError-exit.)

[domenic]

Could you explain your motivation why you don't want to catch errors? What is the issue you are concerned with?

My motivation here is that wrap does not seem like the right place to do this. How errors are handled should be the responsibility of the code calling the function, not the code storing the function to run later. If we say that wrap does both things (tie a function to a zone, and change its error handling behavior) certain patterns like Node.js's event emitter, or promises, are not possible to implement using wrap. Since we expect wrap to be used by libraries with complex scheduling needs anyway, it seems very unclear to me that preventing those libraries from seeing the error and choosing how to deal with it themselves is the correct choice.

[domenic]

A more inclusive approach would be to have all four of wrap+wrapGuarded and run+runGuarded. That might start making this proposal seem too heavy though and like it is dealing with error handling too much. @ofrobots @littledan thoughts?

If we are going for minimal, and for not dealing with error handling (i.e. we have no desire to run error handlers inside a specific zone in the initial proposal), then it really seems like we should let errors bubble instead of sending them to window.onerror.

[ofrobots]

I strongly favor keeping the proposal minimal and staying away from error handling at the moment. Couldn't runGuarded and wrapGuarded be introduced in the future when we are add error handling semantics to the spec. I don't think the semantics of run and wrap need to change at that point, correct?

/cc @matthewloring

[littledan]

I like the minimal proposal better too, leaving out error handling. It seems like implementations of user-level queuing have two different problems--propagating zones and having different error handling semantics. Error handling semantics can be done in JavaScript instead, and don't need built-in support the way that Zones do. It could be a nice convenience for these queuing library authors, but it is not part of the minimal package.

[bmeck]

I have great fear that unexpected errors will be silenced and leave applications in bad state. This was a problem with domains once libraries started to swallow each other's errors.

[mhevery]

(sorry for the delay in getting back to this issue)

From my experience with implementing Zones I think that having wrap not handle errors in a mistake. If we do this we will not be able to change the semantics later. So I would rather just stay out of error handling and wrap method all together.

Proposal: Let's remove wrap from spec. It is easy to polyfill, and user libraries can easily create such a function.

[littledan]

Are you proposing that run would be the only primitive?

[mhevery]

@littledan yes. (there is nothing special about wrap and so it can be done in user space.)

[littledan]

As a language nerd, I always like fewer primitives. However, this may have implications for being efficiently implementable. It might be that a wrap primitive is more efficient than run+closure, but we have not yet done enough implementation design work to say.

[domenic]

@mhevery and I discussed this in some detail and he managed to convince me that most libraries do in fact want wrapGuarded instead of wrap. Since there seems to be a general push against having error handling in the MVP spec, I think the course of action is then to remove wrap.

[littledan]

@domenic could you explain what caused your change of heart here?

[bmeck]

Does this mean more silent errors if handleError is not callable?

[domenic]

What is handleError?

[bmeck]

From #1 (comment)

zoneForF.handleError(e);

That was for runGuarded, but I presume wrapGuarded would be similar.

[domenic]

That's not part of this proposal, it was just a strawman as part of the discussion.

[bmeck]

@domenic then what is wrapGuarded?

[domenic]

It's not clear. That's what this thread was discussing, before we decided to not have any type of wrap at all.