dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.2k stars 1.57k forks source link

Drop the concept of "error zones". #39825

Open lrhn opened 4 years ago

lrhn commented 4 years ago

Currently the Zones from dart:async have a notion of being an "error zone". An error zone is one which has a handler for uncaught errors, and an error "cannot flow between error zones".

The latter is not necessary, it's hard to explain exactly what it means, and it's actively harmful in some cases (like futures which never complete because they are completed with an error future "belonging" to a different error zone).

It's hard to explain that a value belongs to a zone. Zones are something that computations run in, but once a computation has finished, the value does not really belong anywhere. If stored in a variable, anyone can read that variable again from any zone. If stored in a future, that future can be read from anywhere ... except if it's an error, then it behaves unpredictably around error zones. There is no way to check whether a future belongs to a different error zone, but if it does awaiting it will never complete, which causes a number of subtle and hard-to-debug errors.

If you can access a future's error, then you can easily move it into a different error zone manually,

I propose removing the rule that an error cannot flow between error zones. This effectively removes the concept of error zones. The handleUncaughtError is just that, the handler of uncaught errors of computations.

This will not change the behavior of any program which doesn't throw. It will not change the behavior of any program which does not introduce an error zone. It will change the behavior of programs where an error is currently blocked from entering a different error zone. It's unclear whether this is a good or bad thing because it's very unclear where that actually happens. It's not formally specified what it means for an error to "pass between error zones".

@leafpetersen


The current implementation stores the zone of a future in the future when it is created. This includes storing the when creating a completer, and not when actually completing the future. That's somewhat inconsistent, and I'd prefer if we could change that so that it's the zone that actually calls complete which is the zone of the future. If futures need a zone at all.

We can remove the zone from futures completely, but that might change which zone some scheduleMicrotask operations are executed in. It might not be worth it.

Futures also use their zone for one other thing: Scheduling microtasks when:

For the first cases, using the "current zone" which completes the future is a reasonable choice. In most cases, the zone where the completer was created and where it is completed is the same anyway. For the last case, or for scheduling listeners in general, it's also possible to use the zone of the listener callbacks. I'd prefer not to do that because it provides a way to circumvent the asynchronicity of futures.

eernstg commented 4 years ago

Eliminating a concept which has an under-specified and confusing semantics sounds like an improvement, especially if there are no major use cases. A (prototype) implementation and a run on a large body of software could be helpful to clarify that there is indeed no major use case.

lrhn commented 4 years ago

Prototype: https://dart-review.googlesource.com/c/sdk/+/128665 (Failures are expected. I didn't remove the tests).

It's a remarkably small change. Seems like only future propagation checks the error zone at all and not, e.g., stream listeners.

natebosch commented 3 years ago

I hit an edge where it's hard to design a nice API due to this.

In the test runner we'd like to have an API allowing folks to build the behavior of our expectAsync utilities - specifically to force the test to remain open (or timed out as a failure) unless some later condition is met. Internally our API looked like addOutstandingCallback() and removeOutstandingCallback() where we took care to maintain the count carefully. I think a slightly friendlier API is mustWaitFor(Future), where the future should complete to indicate the later condition is met.

The implementation with Future comes with some baggage, which is that if the Future will complete as an error it either must be created in the test's error zone, or we could support futures from the same zone as the call to mustWaitFor if we wanted. What we can't do is be sure that will be aware of a future that completes as an error. Unless I'm missing something, we can't even check for the mistake by confirming that the future was created in an allowed error zone.