Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VM should pause or cancel at a yield if the listener pauses or cancels in response to that event. #34775

Closed
lrhn opened this issue Oct 12, 2018 · 4 comments
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@lrhn
Copy link
Member

lrhn commented Oct 12, 2018

The current specification of async* states that for yield value, the implementation must deliver the value (meaning the stream's subscription's onData handler must be called) before it continues after the yield. If the subscription has been paused or cancelled immediately by that call (synchronously), then the implementation should pause or cancel, respectively, and not continue with the following code.

The VM does continue.
Example:

Stream<int> foo() async* {
  try {
    print("a1");
    yield 1;
    print("a2");
    yield 2;
    print("a3");
    yield 3;
  } finally {
    print("af");
  }
}

void main() {
  var s = foo().listen(null);
  s.onData((int v) {
    print("e$v");
    if (v == 1) {
      var delay = Future.delayed(Duration(seconds: 1)).whenComplete(() {
        print("resume");
      });
      print("pause");
      s.pause(delay);
    } else if (v == 2) {
      s.cancel().whenComplete(() {
        print("ef");
      });
    }
  });
}

This should print in the following order: a1, e1, pause, resume, a2, e2, af, ef

It actually prints: a1, e1, pause, a2, resume, e2, a3, af, ef

That is, neither pause nor cancel is respected until the next yield is reached.

(Dart2js gets half if this right - it does pause in the correct place, but does not cancel at the second yield).

@lrhn lrhn added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) labels Oct 12, 2018
@CosmicPangolin
Copy link

@lrhn
Out of curiosity, is this being addressed anytime soon? :)

@sgrekhov
Copy link
Contributor

sgrekhov commented Jul 6, 2020

Please note that this issue exists for dartk, dartdevk and dartkp as well

nex3 added a commit to nex3/cli_repl that referenced this issue Apr 14, 2021
jathak pushed a commit to jathak/cli_repl that referenced this issue Apr 14, 2021
copybara-service bot pushed a commit that referenced this issue Apr 6, 2022
…active subscription (not paused/cancelled)

This fixes an issue where VM would run the async* generator after a
`yield` / `yield*` even though the subscription may be paused or
cancelled.

Furthermore this fixes an issue where `StackTrace.current` used
in async* generator crashes VM and/or produces truncated stack
trace.

This fixes the following existing tests that were failing before:

  * co19/Language/Statements/Yield_and_Yield_Each/Yield_Each/execution_async_t08
  * co19/Language/Statements/Yield_and_Yield_Each/Yield_Each/execution_async_t09
  * co19/Language/Statements/Yield_and_Yield_Each/Yield_Each/execution_async_t10
  * language/async_star/async_star_cancel_test
  * language/async_star/pause_test

Issue flutter/flutter#100441
Issue #48695
Issue #34775

TEST=vm/dart{,_2}/causal_stacks/flutter_regress_100441_test

Change-Id: I73f7d0b70937a3e3766b992740fa6fe6e6d57cec
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/239421
Reviewed-by: Lasse Nielsen <lrn@google.com>
Commit-Queue: Martin Kustermann <kustermann@google.com>
copybara-service bot pushed a commit that referenced this issue Apr 7, 2022
…there's active subscription (not paused/cancelled)"

This reverts commit 837ee17.

Reason for revert: Please see flutter/flutter#101514

Original change's description:
> [vm] Fix some async* semantics issues: Only run generator if there's active subscription (not paused/cancelled)
>
> This fixes an issue where VM would run the async* generator after a
> `yield` / `yield*` even though the subscription may be paused or
> cancelled.
>
> Furthermore this fixes an issue where `StackTrace.current` used
> in async* generator crashes VM and/or produces truncated stack
> trace.
>
> This fixes the following existing tests that were failing before:
>
>   * co19/Language/Statements/Yield_and_Yield_Each/Yield_Each/execution_async_t08
>   * co19/Language/Statements/Yield_and_Yield_Each/Yield_Each/execution_async_t09
>   * co19/Language/Statements/Yield_and_Yield_Each/Yield_Each/execution_async_t10
>   * language/async_star/async_star_cancel_test
>   * language/async_star/pause_test
>
> Issue flutter/flutter#100441
> Issue #48695
> Issue #34775
>
> TEST=vm/dart{,_2}/causal_stacks/flutter_regress_100441_test
>
> Change-Id: I73f7d0b70937a3e3766b992740fa6fe6e6d57cec
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/239421
> Reviewed-by: Lasse Nielsen <lrn@google.com>
> Commit-Queue: Martin Kustermann <kustermann@google.com>

# Not skipping CQ checks because original CL landed > 1 day ago.

Change-Id: Ic3d9c0508310a33a2aaee67860c0bb2ec83bab4a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/240506
Reviewed-by: Siva Annamalai <asiva@google.com>
Reviewed-by: Ben Konyi <bkonyi@google.com>
Commit-Queue: Siva Annamalai <asiva@google.com>
copybara-service bot pushed a commit that referenced this issue Apr 26, 2022
…there's active subscription (not paused/cancelled)"

This fixes an issue where VM would run the async* generator after a
`yield` / `yield*` even though the subscription may be paused or
cancelled.

Furthermore this fixes an issue where `StackTrace.current` used
in async* generator crashes VM and/or produces truncated stack
trace.

This fixes the following existing tests that were failing before:

  * co19/Language/Statements/Yield_and_Yield_Each/Yield_Each/execution_async_t08
  * co19/Language/Statements/Yield_and_Yield_Each/Yield_Each/execution_async_t09
  * co19/Language/Statements/Yield_and_Yield_Each/Yield_Each/execution_async_t10
  * language/async_star/async_star_cancel_test
  * language/async_star/pause_test

New in reland: Allow the generator to to cause cancelling it's own consumer.
This addresses the issue of original revert
  -> see flutter/flutter#101514

Issue flutter/flutter#100441
Issue #48695
Issue #34775

TEST=vm/dart{,_2}/causal_stacks/flutter_regress_100441_test

Change-Id: I091b7159d59ea15fc31162b4b6b17260d523d7cb
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/242400
Reviewed-by: Lasse Nielsen <lrn@google.com>
Commit-Queue: Martin Kustermann <kustermann@google.com>
@a-siva
Copy link
Contributor

a-siva commented May 12, 2022

Can this issue be closed ?

@lrhn
Copy link
Member Author

lrhn commented May 16, 2022

The VM now behaves correctly on the original example, so LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

4 participants