Don't use `sync` controller for "results" stream.

[floitschG]

When async functions start synchronously, a cycle leads to problems with this synchronous stream controller. We could probably insert delays in other parts of the package, but this looks like a safe place to switch from sync to non-sync.

[floitschG]

@nex3 ping.

[floitschG]

@munificent ping.

[munificent]

I'm sorry, but I'm not qualified to review this. :(

I know @nex3 made it synchronous deliberately, but I don't know why and I don't understand the code well enough to know if changing it will break things. I do remember that she had to be careful about which streams were synchronous and which were not to ensure race conditions couldn't happen.

What kinds of cycle problems are you seeing? Why make the stream asynchronous instead of inserting delays in the code that reads from the stream?

[matanlurey]

Either @natebosch or @jakemac53 might be good backups.

[natebosch]

We could also potentially insert await nulls into async functions to keep old behavior?

@floitschG - how did you track down this specific Stream as a problem?

[floitschG]

I just applied the patch from here: https://dart-review.googlesource.com/5263 and tried to do a pub build.

Here is the error:

Tried calling: send(SendPort)
#0      Object._noSuchMethod (dart:core-patch/object_patch.dart:42)
#1      Object.noSuchMethod (dart:core-patch/object_patch.dart:46)
#2      loadTransformers (package:$pub/transformer_isolate.dart:21:11)
#3      main (file:///tmp/pub_NOJZGF/runInIsolate.dart:4:35)
#4      _startIsolate.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:261)
#5      _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:151)
Building websync... 
Bad state: Cannot fire new event. Controller is already firing an event
dart:async                                                 _BroadcastStreamController.addError
package:barback/src/graph/package_graph.dart 288           PackageGraph._inErrorZone.<fn>
dart:async                                                 _BroadcastStreamController.add
package:barback/src/graph/node_streams.dart 55             NodeStreams.changeStatus
package:barback/src/graph/transformer_classifier.dart 106  TransformerClassifier.addInput.<fn>.<fn>
dart:async                                                 _BroadcastStreamController.add
package:barback/src/graph/node_streams.dart 55             NodeStreams.changeStatus
package:barback/src/graph/transform_node.dart 535          TransformNode._apply
package:barback/src/graph/transform_node.dart 285          TransformNode.force
package:barback/src/graph/transformer_classifier.dart 141  TransformerClassifier.forceAllTransforms
package:barback/src/graph/phase.dart 288                   Phase.forceAllTransforms
package:barback/src/graph/asset_cascade.dart 210           AssetCascade.forceAllTransforms
package:barback/src/graph/package_graph.dart 156           PackageGraph.getAllAssets.<fn>
dart:async                                                 new Future.sync
package:barback/src/graph/package_graph.dart 280           PackageGraph._inErrorZone.<fn>
dart:async                                                 runZoned
package:barback/src/graph/package_graph.dart 279           PackageGraph._inErrorZone
package:barback/src/graph/package_graph.dart 156           PackageGraph.getAllAssets
package:barback/src/graph/package_graph.dart 161           PackageGraph.getAllAssets.<fn>
dart:async                                                 _BroadcastStreamController.add
package:barback/src/graph/package_graph.dart 266           PackageGraph._tryScheduleResult.<fn>
package:barback/src/utils.dart 252                         newFuture
package:barback/src/graph/package_graph.dart 260           PackageGraph._tryScheduleResult
package:barback/src/graph/package_graph.dart 95            new PackageGraph.<fn>.<fn>
dart:async                                                 _BroadcastStreamController.add
package:barback/src/graph/node_streams.dart 55             NodeStreams.changeStatus
dart:async                                                 _BroadcastStreamController.add
package:barback/src/graph/node_streams.dart 55             NodeStreams.changeStatus
package:barback/src/graph/phase.dart 256                   Phase.updateTransformers.<fn>
dart:async                                                 _BroadcastStreamController.add
package:barback/src/graph/node_streams.dart 55             NodeStreams.changeStatus
package:barback/src/graph/transformer_classifier.dart 120  TransformerClassifier.addInput.<fn>
This is an unexpected error. Please run

    pub --trace build

and include the logs in an issue on https://github.com/dart-lang/pub/issues/new

[natebosch]

The commit which switched this to synchronous is huge and doesn't have much description. I think this was only done as an optimization and should be safe to make it async, but it's hard to tell. I think it's almost certainly breaking the rules in it's current usage so it should probably be changed. Unfortunately I don't have time to for due diligence on this until I'm back from my trip. Perhaps @jakemac53 can dig further, or if it can wait a couple weeks I can take a closer look.

[floitschG]

Fwiw, I actually think that the way the sync-controller is used, is probably correct here. I could only see tail-calls from async calls. The bad thing seems that it ends up calling itself again (which is ok, if there is a delay from an async).

I love to complain about developers not respecting our guidelines for sync streams, but this time I couldn't see anything wrong.

[jakemac53]

@jakemac53 is hiding from this one since he doesn't know enough about barback implementation details or sync streams to be of much use.

[natebosch]

I think there is a fully synchronous path to this:

• PackageGraph constructor
• _inErrorZone with a synchronous body callback
• runZoned with a Future.sync(body) that throws from the catchError
• I think that this can then synchronously reach the onError from runZoned so we'd be synchronously adding an error to this stream.

Maybe this is OK since we wouldn't be able to reach the field from somewhere else since the constructor isn't finished? Maybe the catchError introduces async behavior even though it's on a Future.sync?

[natebosch]

Looks like there are other paths to _inErrorZone from synchronous calls other than the constructor, so I think the addError calls can be synchronous in other places too.

Perhaps what we need to look at is the newFuture thing? I never fully understood what it was for, but based on the referenced bug maybe we can replace it with new Future.microtask() to keep the old behavior?

https://github.com/dart-lang/barback/blob/7ff1e1d31e21f71d920b71065f76ce6a252b153c/lib/src/utils.dart#L246

In this case I think what were effectively doing is:

void someMethod() {
  () async {
    await _resultStream.add(...);
  }
}

Which by the new semantics of async would be breaking the rules?

[natebosch]

Can you try https://github.com/dart-lang/barback/pull/91 with your change in the SDK and see if it fixes things? I think it's a more minimally invasive change since it should (I think?) keep the same behavior as today.

[floitschG]

Closing, since #91 is superseding this one.