[Breaking change request] The runZoned method will now not catch synchronously thrown errors even when given an error handler.

[lrhn]

2020-03-23 - Rewritten to state the actual change decided upon.

Summary

The runZoned method will not have an onError parameter. A runZonedGuarded function is added which has an onError parameter.

The IOOverrides and HttpOverrides static run* methods will not have onError or zoneSpecification parameters.

What is changing:

The runZoned method is declared as

R runZoned<R>(
  R body(), {
  Map zoneValues,
  ZoneSpecification zoneSpecification
})

and we add:

R? runZonedGuarded<R>(
  R body(), 
  void onError(Object error, StackTrace stack), {
  Map zoneValues,
  ZoneSpecification zoneSpecification,
})

The static methods from dart:io have their zoneSpecification and onError parameters removed.

Why is this changing?

The existing uses of runZoned can be split into two groups: Those with onError and those without.
Those without an onError always return the result of calling body. Those with onError return either that or null.

In Null Safe code, using the same function for both operations would require the return type to always be nullable, which makes the first group unnecessarily cumbersome.
Other workarounds are either fragile or surprising (for example, as originally proposed, throwing if the R type is not nullable and returning null if not).

This change is larger, but ends up with a better API for Null Safety.

The dart:io override classes would have to split their static run* methods in the same way to be compatible. Instead it was decided that the onError and zoneSpecification parameters were not used often enough for them to carry their own weight. Removing them gives a better API, and users can still use the dart:async runZoned/runZonedGuarded methods as well, if necessary.

Expected impact

Code which uses runZoned with an onError argument to catch synchronous errors will need to call runZonedGuarded instead.

Migration

For now, the onError parameter is only @deprecated, and in the Null Safe SDK is made to throw if catching a synchronous error and the return type is not nullable. Existing code should keep running until it is migrated to using runZonedGuarded.
When enough code has been migrated off the deprecated member, it will be removed entirely in a separate CL.

[franklinyow]

cc @Hixie @matanlurey @dgrove @vsmenon for review and approval.

[Hixie]

It's not really clear to me when you would intentionally use onError with runZoned and expect a synchronous return value, other than the specific case of wanting to have the handler handle the error and the call to return null.

[lrhn]

@Hixie Not sure I understand.

The current uses of runZoned fall into a number of use-cases:

• Non-error zone: No onError and no handleUncaughtError in the zone specification (if any). The call just returns the result of calling body in a zone with some other non-error zone features used.
• Error zone for async errors: The body always returns a value, or it is even void, and the error handler is just for async errors. The onError is used as a shorthand if no other zone feature in the zone specification needs to be set.
• Preventing all errors from escaping: The onError is used to catch all errors, synchronous and asynchronous, so that the call to runZoned(body ...) never causes an error.

The third item is the one that we cannot support with null safe types without making the return type nullable (R? instead of R). Because we might not be able to return null, we might have to throw synchronously anyway, which breaks that use-case.
The second use-case is unaffected because it never needs to return null as a default value.

If we change the return type to R? then we make the first two use-cases more complicated even though they don't use the synchronous error capture. That's a bad design because it puts the cost of the feature on the code that doesn't use that feature.

In languages with overloading, it would not be a problem because they would just have a version without onError returning R and a version with onError returning R?. We can't do that.

We could introduce a second R? runZonedGuarded<R>(...same arguments...), possibly onError as positional required parameter, which works like the current runZoned and captures sync errors and returns null. Then we can migrate code needing the behavior to that function instead. That can be used as breakage mitigation, it doesn't change that we are breaking the behavior of the existing runZoned.

[Hixie]

If it was up to me I think my proposal would be to deprecate runZoned entirely and have two alternatives, one without error handling that returns a non-nullable value, and one with error handling that returns a nullable value.

That said, it seems Zone already has a more elaborate API here. Indeed, runZoned and Zone.run, Zone.runGuarded, et al, are quite different in semantics and that seems unnecessarily inconsistent. I think we should probably just rationalize these two APIs so that they are equivalent. If we do that, then we can simultaneously make sure that our new API is rational in an NNBD world (e.g. looks like Zone.runGuarded returns void, not R, and so side-steps the problem; why can't we do the same here?).

[lrhn]

We can do a lot of things if we want to change or replace runZoned.

The one thing we cannot do is to keep it working like it does now (sometimes returning null) without making the API bad for users who just want the result of their non-throwing body function.

That does suggest another approach: Make the return type R?, which is highly annoying for people who don't need the null, but provide an alternative function for those that they can migrate to instead.

So:

/// Keeps working like now. Perhaps deprecate it.
R? runZoned<R>(R body(), { ... }) ...

/// Captures all errors, sync or async. Returns nothing.
void runZonedGuarded(void body(), {..., void onError(Object error, StackTrace trace)})  ...

/// Captures async errors only
R runZonedWith(R body(), { ..., void onError(Object error, StackTrace trace)}) ...

[Hixie]

runZonedWith would be inconsistent with the names on Zone, which seems like something to avoid. Also it seems weird to only catch async errors. I don't understand that use case. As far as I can tell, that's also not something any of the Zone methods do.

How about:

/// Captures all errors, sync or async. Returns nothing.
void runZonedGuarded(void body(), {..., void onError(Object error, StackTrace trace)})  ...

/// Does not capture errors (no error handler).
R runZoned(R body(), { ... }) ...

If I understand the Zone API correctly, that would at least be consistent with Zone's runGuarded and run.

[lrhn]

I like the APIs. It's what I was going for above, but without keeping the legacy function around. That's also the biggest issue.

This would be a hard breaking change since the runZoned function can no longer be called with onError. All existing code calling runZoned with an onError needs to be migrated.

That suggests a two-step migration:

• First introduce runZoneGuarded as defined above in both legacy and NNBD libraries (probably make onError a required positional parameter, because it isn't really guarded without one). Deprecate onError on runZoned.
• In the NNBD SDK only, remove the onError from runZoned.

The we won't remove the onError until we release the unforked core librarires. That gives people time to migrate their code, and if they don't, they will be forced to when we unfork the libraries.

(The runZoned here would be able to catch only async errors by providing a ZoneSpecifiction with an uncaughtErrorHander. That's how error zones usually work. I also think we should remove the error zone concept.)

[Hixie]

LGTM.

[vsmenon]

lgtm for dart

[matanlurey]

I don't have a strong opinion, though we have a ton of magic "Zone" code throughout google3; in fact I see at least 200+ occurrences. I would feel better like Hixie mentioned if we had runZoneGuarded as an easy "swap to the old behavior" in many cases.

[franklinyow]

Approved

[lrhn]

That's two votes for runZonedGuarded.

I'll wrap up a CL for that (https://dart-review.googlesource.com/c/sdk/+/137302) with an API like:

R runZoned<R>(
    R body(), 
   {ZoneSpecification zoneSpecification, 
    ZoneVariables zoneValues}) { ... }

R? runZonedGuarded<R>(
    R body(), 
    Function onError, 
   {ZoneSpecification zoneSpecification, 
    ZoneVariables zoneValues}) { ... }

as the NNBD API, and backported as

R runZoned<R>(
    R body(), 
   {ZoneSpecification zoneSpecification, 
    ZoneVariables zoneValues, 
    @Deprecated("Use runZonedGuarded instead') 
    Function onError}) { ... }

R? runZonedGuarded<R>(
    R body(), 
    Function onError, 
   {ZoneSpecification zoneSpecification, 
    ZoneVariables zoneValues}) { ... }

I agree that it is a better API.

It's a potentially more breaking change. Looking through the SDK alone, there are numerous places which needs to be migrated. I count 16 files in 9 packages in the SDK pkg/ directory which are using runZoned. There are 49 files in the SDK third_party/ directory. Then there are some in runtime/ and sdk/lib/io/.

I fear that migration will be very expensive.

The function which corresponds to the current usage is named differently, so migration has to happen before the platform library unforking.
It's not subtly breaking, which can be an advantage.

[lrhn]

@Hixie Was your LGTM for the original proposal, or for the alternative with runZoned/runZonedGuarded? If the latter, then I'm not sure it counts towards approval for the original.
(In other words: @franklinyow, I'm not entirely sure what has been approved here)

[leafpetersen]

@Hixie @matanlurey Do I understand this as you are both in support of the API proposed here?

cc @natebosch

[natebosch]

One thing that still isn't clear to me with either proposal:

The ZoneSpecification has handleUncaughtError and errorCallback arguments. I don't know the specific use cases between onError, ZoneSpecification.handleUncaughtError, or ZoneSpecification.errorCallback. I don't know how they interact if more than one is specified. And I don't know whether removing onError is sufficient for to allow a non-nullable R return type, or if the ZoneSpecification callbacks could interfere with that.

[lrhn]

The Zone's handleUncaughtError is invoked with any unhandled asynchronous errors.
That is, if a future completes with an error before a listener has been added to it, the error is reported to the handleUncaughtError handler. Similarly if an event throws to the event loop, the zone running the event gets the uncaught error.
And finally, if an error attempts to cross an error zone boundary, it instead goes to the original error zone's uncaught error handler (and the targeted receiver probably never completes, which is bloody annoying and why I want to get rid of the error zone concept completely).

The errorCallback zone function is invoked whenever a synchronous error becomes asynchronous, which means when someone calls the Completer.completeError or Stream.addError methods, or any similar way to inject a user controlled error into the async system. You can also think of it as when the error officially enters an async zone. At that point, the errorCallback gets a chance to intercept and replace the error (perhaps wrap it with more information, or just log it). It's not error handling, the error (or its replacement) continues being thrown afterwards.

The runZoned uses onError for two things: As the uncaughtErrorHandler of the zone and to handle synchronous errors. Since onError has no return value (and it really shouldn't for the uncaught error handler), that leaves the function without a return value if the body throws. Which is why we need to be able to return null instead, it's the only reasonable default value.

With no onError, there is no handling of synchronous errors, and we can complete the runZoned call in the same way the body call completed, whether it's a return value or an error thrown. So, we can keep the same return type as body.

That's why the R runZoned<R>(R body(), {spec, values}) and R? runZonedGuarded<R>(R body(), onError, {spec, values}) distinction gives the most precise return type in either situation.
(Other languages would just use overloading, we need to give them different names).

[natebosch]

Since runZonedGuarded is a new API, I wonder if we should also consider #34144 and use a more specific type than Function.

[lrhn]

@natebosch That is a very good idea.
I really wish we'd just made all error handlers be Function(Object, StackTrace). It would disallow code like something.catchError(print), but you only ever use that for testing anyway.

[lrhn]

I just noticed that the HttpOverrides class uses runZoned and forwards the onError directly.
The NNBD version of that fails when switching to runZonedGuarded with a nullable return type.

That API should proably also be changed to runZoned/runZonedGuarded(/runWithHttpOverrides/runWithHttpOverridesGuarded).
Alternatively it should stop providing the onError parameter entirely (and zoneSpecification too), and just allow you to run with overrides instead of being an alternative to the general runZoned function. I think I'd recommend that, but that too will be a breaking change.

[natebosch]

Alternatively it should stop providing the onError parameter entirely (and zoneSpecification too), and just allow you to run with overrides instead of being an alternative to the general runZoned function. I think I'd recommend that, but that too will be a breaking change.

I would strongly prefer this. The API is considerably less confusing if we don't try to mix responsibilities. (I'd also prefer we hadn't add 2 flavors of the API just for HttpOverrides, but oh well).

We aren't going to break much, if any, code removing these arguments. Usage is incredibly low. I cannot find a single call to either API that includes these arguments.

[lrhn]

The new proposal is now available as https://dart-review.googlesource.com/c/sdk/+/137302

It breaks building the SDK in NNBD mode because of the removed onError in runZoned, and the
SDK depending on package:stack_trace which uses that. So, we need to migrate a number of packages (at least one known so far) to using runZonedGuarded before we can unfork the SDK.

[lrhn]

I have updated the documentation in this issue to match the CL.

[franklinyow]

Change landed