Fix spurious resume when awaiting future in async* code

Fix spurious resume when awaiting future in async* code

Fixes issue reported in bug 23116.

The equivalent fix for dart2js contains a test case for this.

R=iposva@google.com

Committed: https://code.google.com/p/dart/source/detail?r=44985

[hausner, 2015-04-08 21:26:46]

hausner@google.com changed reviewers:
+ iposva@google.com

[hausner, 2015-04-08 21:27:46]

The code in parser.cc comes in handy when debugging auto-generated code. It's
not used for production.

[Ivan Posva, 2015-04-08 21:49:55]

LGTM -ip

https://codereview.chromium.org/1073623002/diff/1/runtime/lib/core_patch.dart
File runtime/lib/core_patch.dart (right):

https://codereview.chromium.org/1073623002/diff/1/runtime/lib/core_patch.dart...
runtime/lib/core_patch.dart:67: // if (controller.isCanelled) return;
Cannelloni?

[hausner, 2015-04-08 22:09:27]

Committed patchset #2 (id:20001) manually as r44985 (presubmit successful).

[hausner, 2015-04-08 22:12:45]

Thanks.

https://codereview.chromium.org/1073623002/diff/1/runtime/lib/core_patch.dart
File runtime/lib/core_patch.dart (right):

https://codereview.chromium.org/1073623002/diff/1/runtime/lib/core_patch.dart...
runtime/lib/core_patch.dart:67: // if (controller.isCanelled) return;
On 2015/04/08 21:49:55, Ivan Posva wrote:
> Cannelloni?

Basta.

[Lasse Reichstein Nielsen, 2015-04-09 06:35:29]

lrn@google.com changed reviewers:
+ lrn@google.com

[Lasse Reichstein Nielsen, 2015-04-09 06:35:30]

lgtm

https://codereview.chromium.org/1073623002/diff/20001/runtime/lib/core_patch....
File runtime/lib/core_patch.dart (right):

https://codereview.chromium.org/1073623002/diff/20001/runtime/lib/core_patch....
runtime/lib/core_patch.dart:46: isSuspendedAtYield = false;
If I understand this correctly, your possible states are:
- scheduled/running (some behavior depends on whether it's canceled)
- adding
- suspended
- done (reached end of function)
While running, even with async code, the async-star controller isn't invoked.
Only when you reach a yield, yield* or exit of the function will the controller
be used.

At a yield*, the state transitions to "adding". When the addStream completion
Future completes, state transitions to scheduled/running (there is no cancel or
pause check after a yield*, only before).

At a yield:
- if canceled, transition to scheduled (and try to exit).
- if paused, transition to suspended.
- otherwise, transition to scheduled (keep running)

On a resume, if suspended, transition to scheduled.

On a cancel, if suspended, transition to scheduled (and set the "cancelled" flag
so yields start trying to exit).

When exiting function, transition to done.


The boolean flags matches this (I think), but if the code was more explicit
about these states, I think the code would be easier to reason about (at least
for me :).

[hausner, 2015-04-09 17:10:19]

https://codereview.chromium.org/1073623002/diff/20001/runtime/lib/core_patch....
File runtime/lib/core_patch.dart (right):

https://codereview.chromium.org/1073623002/diff/20001/runtime/lib/core_patch....
runtime/lib/core_patch.dart:46: isSuspendedAtYield = false;
On 2015/04/09 06:35:30, Lasse Reichstein Nielsen wrote:
> If I understand this correctly, your possible states are:
> - scheduled/running (some behavior depends on whether it's canceled)
> - adding
> - suspended
> - done (reached end of function)
> While running, even with async code, the async-star controller isn't invoked.
> Only when you reach a yield, yield* or exit of the function will the
controller
> be used.
> 
> At a yield*, the state transitions to "adding". When the addStream completion
> Future completes, state transitions to scheduled/running (there is no cancel
or
> pause check after a yield*, only before).
> 
> At a yield:
> - if canceled, transition to scheduled (and try to exit).
> - if paused, transition to suspended.
> - otherwise, transition to scheduled (keep running)
> 
> On a resume, if suspended, transition to scheduled.
> 
> On a cancel, if suspended, transition to scheduled (and set the "cancelled"
flag
> so yields start trying to exit).
> 
> When exiting function, transition to done.
> 
> 
> The boolean flags matches this (I think), but if the code was more explicit
> about these states, I think the code would be easier to reason about (at least
> for me :).


I agree that the code and comments could be clearer about the various states. I
don't have a clear picture of the various events and states that streams,
controllers, listeners etc can be in, so this code is more ad-hoc than I'd like.

[kasperl, 2015-04-10 08:27:18]

kasperl@google.com changed reviewers:
+ kasperl@google.com

[kasperl, 2015-04-10 08:27:18]

No tests?

[kasperl, 2015-04-10 08:28:47]

Let me rephrase that: I think it would have been nice to submit this with at
least the test cases from the dart2js implementation.

[hausner, 2015-04-10 15:42:17]

As Ivan mentioned, I didn't want to "steal" the test from the dart2js CL. I did
run that test as a one-off to check this CL.