Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1070733002:
Dart2js async* functions only resume execution of the body when suspended on yield. (Closed)
|
Created:
5 years, 8 months ago by sigurdm Modified:
5 years, 8 months ago CC:
reviews_dartlang.org, hausner Target Ref:
refs/remotes/git-svn Visibility:
Public. |
DescriptionDart2js async* functions only resume execution of the body when suspended on yield.
Before they would also resume if suspended on await, leading to resuming
on unfinished futures.
BUG= dartbug.com/23129
R=floitsch@google.com, lrn@google.com
Committed: https://code.google.com/p/dart/source/detail?r=45056
Patch Set 1 #
Total comments: 21
Patch Set 2 : Address review comments #Patch Set 3 : Second upload #Patch Set 4 : Rewrite addressing points from Lasse #
Total comments: 16
Patch Set 5 : Address review comments #
Total comments: 4
Patch Set 6 : Address review #
Messages
Total messages: 12 (1 generated)
sigurdm@google.com changed reviewers: + floitsch@google.com, lrn@google.com
LGTM. https://codereview.chromium.org/1070733002/diff/1/sdk/lib/_internal/compiler/... File sdk/lib/_internal/compiler/js_lib/js_helper.dart (right): https://codereview.chromium.org/1070733002/diff/1/sdk/lib/_internal/compiler/... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3801: bool isWaitingForYield = false; isAtYield ? Add comment what exactly this means. For example. /// True, when the async* function just yielded a value, but hasn't /// resumed execution yet. https://codereview.chromium.org/1070733002/diff/1/tests/language/async_star_r... File tests/language/async_star_regression_23116_test.dart (right): https://codereview.chromium.org/1070733002/diff/1/tests/language/async_star_r... tests/language/async_star_regression_23116_test.dart:22: // Now foo will be waiting future. // At this moment foo is waiting on the given future. https://codereview.chromium.org/1070733002/diff/1/tests/language/async_star_r... tests/language/async_star_regression_23116_test.dart:24: // Ensure that execution of foo is not resumed - future is not completed yet. the future
https://codereview.chromium.org/1070733002/diff/1/sdk/lib/_internal/compiler/... File sdk/lib/_internal/compiler/js_lib/js_helper.dart (right): https://codereview.chromium.org/1070733002/diff/1/sdk/lib/_internal/compiler/... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3801: bool isWaitingForYield = false; On 2015/04/08 14:01:25, floitsch wrote: > isAtYield ? > > Add comment what exactly this means. For example. > /// True, when the async* function just yielded a value, but hasn't > /// resumed execution yet. Done. https://codereview.chromium.org/1070733002/diff/1/tests/language/async_star_r... File tests/language/async_star_regression_23116_test.dart (right): https://codereview.chromium.org/1070733002/diff/1/tests/language/async_star_r... tests/language/async_star_regression_23116_test.dart:22: // Now foo will be waiting future. On 2015/04/08 14:01:25, floitsch wrote: > // At this moment foo is waiting on the given future. Done. https://codereview.chromium.org/1070733002/diff/1/tests/language/async_star_r... tests/language/async_star_regression_23116_test.dart:24: // Ensure that execution of foo is not resumed - future is not completed yet. On 2015/04/08 14:01:25, floitsch wrote: > the future Done.
https://codereview.chromium.org/1070733002/diff/1/sdk/lib/_internal/compiler/... File sdk/lib/_internal/compiler/js_lib/js_helper.dart (right): https://codereview.chromium.org/1070733002/diff/1/sdk/lib/_internal/compiler/... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3750: // TODO(sigurdm): We should not suspend here according to the spec. Isn't the spec allowing you to do so, but not requiring it (as long as you "eventually" break a loop of yields)? I think it's the right thing to do - output the value, allow the listener to process it, then go back to creating the next value. https://codereview.chromium.org/1070733002/diff/1/sdk/lib/_internal/compiler/... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3752: controller.isWaitingForYield = false; I think this line should be moved outside of the scheduleMicrotask. At the time you do the scheduleMicrotask, you have already decided that you are not paused, and execution will continue. Alternatively, as written below, do the isPaused check in the scheduleMicrotask callback. https://codereview.chromium.org/1070733002/diff/1/sdk/lib/_internal/compiler/... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3756: }); I think I would prefer waiting until the scheduleMicrotask callback to check if the output stream is paused. That allows listeners on the output stream to pause in response to the just output value, and have it immediately stop working. It also avoids the race condition of the subscription not being paused in the controller.isPaused text above, but becoming paused *and* resumed again before the scheduleMicrotask callback is run. I think that would still cause multiple resumes below. https://codereview.chromium.org/1070733002/diff/1/sdk/lib/_internal/compiler/... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3762: // [AsyncStreamController.addStream]. Missing end-paren :) https://codereview.chromium.org/1070733002/diff/1/sdk/lib/_internal/compiler/... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3801: bool isWaitingForYield = false; Consider documenting these fields, preferably by describing the possible states that they controller can be in (including any scheduleMicrotask callbacks that may be scheduled in some states). I find that being extremely explicit about the states is the best way to avoid an accidental inconsistent state. https://codereview.chromium.org/1070733002/diff/1/sdk/lib/_internal/compiler/... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3823: if (!isAdding && isWaitingForYield) { Maybe name it "isWaitingForResume"? You have already executed the yield, and is now blocked after the yield because the output stream subscription is paused. Should isWaitingForYield be set to false somewhere? https://codereview.chromium.org/1070733002/diff/1/sdk/lib/_internal/compiler/... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3823: if (!isAdding && isWaitingForYield) { I don't think this is safe enough. If the stream becomes both paused after setting isWaitingForYield to true, but before the following scheduleMicrotask callback being run, and it is also resumed again in that time, then this code will run *as well* as the scheduleMicrotask callback. https://codereview.chromium.org/1070733002/diff/1/sdk/lib/_internal/compiler/... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3829: if (isPaused) asyncStarHelper(null, body, this); Are you sure this is safe? You can have a call to asyncStarHelper already scheduled with scheduleMicrotask, and then two rapid calls to sub.pause() and sub.cancel() would end up here, scheduling another continuation. It's generally not clear how you determine if a call to asyncStarHelper is already scheduled somehow (schedule microtask/waiting on future), which is what is really important before scheduling another call.
I did some simplifying changes. PTAL https://codereview.chromium.org/1070733002/diff/1/sdk/lib/_internal/compiler/... File sdk/lib/_internal/compiler/js_lib/js_helper.dart (right): https://codereview.chromium.org/1070733002/diff/1/sdk/lib/_internal/compiler/... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3750: // TODO(sigurdm): We should not suspend here according to the spec. On 2015/04/08 14:12:14, Lasse Reichstein Nielsen wrote: > Isn't the spec allowing you to do so, but not requiring it (as long as you > "eventually" break a loop of yields)? > I think it's the right thing to do - output the value, allow the listener to > process it, then go back to creating the next value. Yes I think so - this is an outdated comment. Removed. https://codereview.chromium.org/1070733002/diff/1/sdk/lib/_internal/compiler/... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3752: controller.isWaitingForYield = false; On 2015/04/08 14:12:15, Lasse Reichstein Nielsen wrote: > I think this line should be moved outside of the scheduleMicrotask. At the time > you do the scheduleMicrotask, you have already decided that you are not paused, > and execution will continue. Alternatively, as written below, do the isPaused > check in the scheduleMicrotask callback. Done. https://codereview.chromium.org/1070733002/diff/1/sdk/lib/_internal/compiler/... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3756: }); On 2015/04/08 14:12:14, Lasse Reichstein Nielsen wrote: > I think I would prefer waiting until the scheduleMicrotask callback to check if > the output stream is paused. That allows listeners on the output stream to pause > in response to the just output value, and have it immediately stop working. > > It also avoids the race condition of the subscription not being paused in the > controller.isPaused text above, but becoming paused *and* resumed again before > the scheduleMicrotask callback is run. I think that would still cause multiple > resumes below. Done. https://codereview.chromium.org/1070733002/diff/1/sdk/lib/_internal/compiler/... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3762: // [AsyncStreamController.addStream]. On 2015/04/08 14:12:15, Lasse Reichstein Nielsen wrote: > Missing end-paren :) Thanks! https://codereview.chromium.org/1070733002/diff/1/sdk/lib/_internal/compiler/... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3801: bool isWaitingForYield = false; On 2015/04/08 14:12:14, Lasse Reichstein Nielsen wrote: > Consider documenting these fields, preferably by describing the possible states > that they controller can be in (including any scheduleMicrotask callbacks that > may be scheduled in some states). > I find that being extremely explicit about the states is the best way to avoid > an accidental inconsistent state. Good advice! I realized I can leave out `isAdding` it contains no useful information beyond isSuspended. Similarly i maintained my own isPaused to be able to decide if to resume after onCancel. That should depend only on isSuspended. https://codereview.chromium.org/1070733002/diff/1/sdk/lib/_internal/compiler/... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3823: if (!isAdding && isWaitingForYield) { On 2015/04/08 14:12:15, Lasse Reichstein Nielsen wrote: > Maybe name it "isWaitingForResume"? You have already executed the yield, and is > now blocked after the yield because the output stream subscription is paused. It is now isSuspended > > Should isWaitingForYield be set to false somewhere? https://codereview.chromium.org/1070733002/diff/1/sdk/lib/_internal/compiler/... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3829: if (isPaused) asyncStarHelper(null, body, this); On 2015/04/08 14:12:14, Lasse Reichstein Nielsen wrote: > Are you sure this is safe? > You can have a call to asyncStarHelper already scheduled with scheduleMicrotask, > and then two rapid calls to sub.pause() and sub.cancel() would end up here, > scheduling another continuation. > > It's generally not clear how you determine if a call to asyncStarHelper is > already scheduled somehow (schedule microtask/waiting on future), which is what > is really important before scheduling another call. True - I need a better mechanism for this. Now I only ever schedule a new call if isSuspended.
https://codereview.chromium.org/1070733002/diff/60001/sdk/lib/_internal/compi... File sdk/lib/_internal/compiler/js_lib/js_helper.dart (right): https://codereview.chromium.org/1070733002/diff/60001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3738: if (controller.cancelationCompleter != null) { if (controller.isCanceled) { ? https://codereview.chromium.org/1070733002/diff/60001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3741: (null); Odd linebreak. Could (null) be on the previous line? https://codereview.chromium.org/1070733002/diff/60001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3767: // demands checks for each element in [stream] not after the last one. ... demands checks *before* each element ... https://codereview.chromium.org/1070733002/diff/60001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3798: /// - canceled I guess the "done" state is implicit. https://codereview.chromium.org/1070733002/diff/60001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3807: /// Or if it has yielded a incomplete sentence. https://codereview.chromium.org/1070733002/diff/60001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3844: // Only schedule again if the async* function actually os suspended. os -> is https://codereview.chromium.org/1070733002/diff/60001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3849: _resumeBody(); I think you still need to do a scheduleMicrotask (doing real work in an onResume callback is not a good idea), but the isSuspended should be cleared immediately. You do go to the running state right here, it's just not starting to run immediately. That will still get out of the suspended state and continue until the next yield. https://codereview.chromium.org/1070733002/diff/60001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3858: scheduleMicrotask(_resumeBody); Shouldn't the body be called with async_error_codes.STREAM_WAS_CANCELED? I think the _resumeBody calls it with .SUCCESS.
https://codereview.chromium.org/1070733002/diff/60001/sdk/lib/_internal/compi... File sdk/lib/_internal/compiler/js_lib/js_helper.dart (right): https://codereview.chromium.org/1070733002/diff/60001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3738: if (controller.cancelationCompleter != null) { On 2015/04/09 12:22:09, Lasse Reichstein Nielsen wrote: > if (controller.isCanceled) { > ? Done. https://codereview.chromium.org/1070733002/diff/60001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3741: (null); On 2015/04/09 12:22:09, Lasse Reichstein Nielsen wrote: > Odd linebreak. > Could (null) be on the previous line? It is to emphasize the call of the returned function. https://codereview.chromium.org/1070733002/diff/60001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3767: // demands checks for each element in [stream] not after the last one. On 2015/04/09 12:22:09, Lasse Reichstein Nielsen wrote: > ... demands checks *before* each element ... Done. https://codereview.chromium.org/1070733002/diff/60001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3798: /// - canceled On 2015/04/09 12:22:09, Lasse Reichstein Nielsen wrote: > I guess the "done" state is implicit. Yes - it corresponds to controller.isClosed https://codereview.chromium.org/1070733002/diff/60001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3807: /// Or if it has yielded a On 2015/04/09 12:22:09, Lasse Reichstein Nielsen wrote: > incomplete sentence. Thanks https://codereview.chromium.org/1070733002/diff/60001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3844: // Only schedule again if the async* function actually os suspended. On 2015/04/09 12:22:09, Lasse Reichstein Nielsen wrote: > os -> is Done. https://codereview.chromium.org/1070733002/diff/60001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3849: _resumeBody(); On 2015/04/09 12:22:09, Lasse Reichstein Nielsen wrote: > I think you still need to do a scheduleMicrotask (doing real work in an onResume > callback is not a good idea), but the isSuspended should be cleared immediately. > You do go to the running state right here, it's just not starting to run > immediately. > That will still get out of the suspended state and continue until the next > yield. Ah - now I understand what you meant. I thought it sounded a bit wierd too. Done https://codereview.chromium.org/1070733002/diff/60001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3858: scheduleMicrotask(_resumeBody); On 2015/04/09 12:22:09, Lasse Reichstein Nielsen wrote: > Shouldn't the body be called with async_error_codes.STREAM_WAS_CANCELED? > I think the _resumeBody calls it with .SUCCESS. Good catch - Done
Lasse - is this one good to go?
lgtm https://codereview.chromium.org/1070733002/diff/80001/sdk/lib/_internal/compi... File sdk/lib/_internal/compiler/js_lib/js_helper.dart (right): https://codereview.chromium.org/1070733002/diff/80001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3768: // one. Should you check for being cancelled? The spec says to check for being canceled *after* adding each element to the output stream (like isPaused is checked before). The addStream may have stopped because the output stream was cancelled. The spec does not expect that to happen automatically, rather it should happen as the first part of the finally-handler that you skip to when seeing that the output stream is cancelled, so you should probably check if the output is cancelled and use .STREAM_WAS_CANCELED as argument instead of .SUCCESS if it is - since you have already closed the input stream. https://codereview.chromium.org/1070733002/diff/80001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3771: (null); This way of emphasizing the call doesn't work for me, it's just confusing :) How about either: (_wrapJsFunctionForAsync(bodyFunctionOrErrorCode, async_error_codes.SUCCESS))(null); or: var wrapped = _wrapJsFunctionForAsync(bodyFunctionOrErrorCode, async_error_codes.SUCCESS); wrapped(null);
https://codereview.chromium.org/1070733002/diff/80001/sdk/lib/_internal/compi... File sdk/lib/_internal/compiler/js_lib/js_helper.dart (right): https://codereview.chromium.org/1070733002/diff/80001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3768: // one. On 2015/04/10 08:22:55, Lasse Reichstein Nielsen wrote: > Should you check for being cancelled? > The spec says to check for being canceled *after* adding each element to the > output stream (like isPaused is checked before). > > The addStream may have stopped because the output stream was cancelled. > The spec does not expect that to happen automatically, rather it should happen > as the first part of the finally-handler that you skip to when seeing that the > output stream is cancelled, so you should probably check if the output is > cancelled and use .STREAM_WAS_CANCELED as argument instead of .SUCCESS if it is > - since you have already closed the input stream. Done. https://codereview.chromium.org/1070733002/diff/80001/sdk/lib/_internal/compi... sdk/lib/_internal/compiler/js_lib/js_helper.dart:3771: (null); On 2015/04/10 08:22:55, Lasse Reichstein Nielsen wrote: > This way of emphasizing the call doesn't work for me, it's just confusing :) > > How about either: > (_wrapJsFunctionForAsync(bodyFunctionOrErrorCode, > async_error_codes.SUCCESS))(null); > or: > var wrapped = _wrapJsFunctionForAsync(bodyFunctionOrErrorCode, > async_error_codes.SUCCESS); > wrapped(null); Done.
Message was sent while issue was closed.
Committed patchset #6 (id:100001) manually as 45056 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
