Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18788002:
Let completers complete in the zone they were constructed in. (Closed)
|
Created:
7 years, 5 months ago by floitsch Modified:
7 years, 5 months ago CC:
reviews_dartlang.org, Anders Johnsen Visibility:
Public. |
DescriptionLet completers complete in the zone they were constructed in.
Makes zones work with IO.
Unfortunately the zones currently don't know when they are done.
I need to investigate this.
R=ajohnsen@google.com, lrn@google.com
Committed: https://code.google.com/p/dart/source/detail?r=24947
Patch Set 1 #
Total comments: 19
Patch Set 2 : Remove zone from completer. #
Total comments: 14
Patch Set 3 : Address comments. #Patch Set 4 : Fix after rebase. #
Total comments: 4
Patch Set 5 : Move comment to correct place. #Messages
Total messages: 13 (0 generated)
LGTM, with a few comments. I'm not sure about some of them, I'm not so familiar with this territory anymore. https://codereview.chromium.org/18788002/diff/1/sdk/lib/async/future_impl.dart File sdk/lib/async/future_impl.dart (right): https://codereview.chromium.org/18788002/diff/1/sdk/lib/async/future_impl.dar... sdk/lib/async/future_impl.dart:11: final Future<T> future; It's okay to type this _FutureImpl, and then avoid the casts in the methods, as this is not exposed (it's the private _Completer). https://codereview.chromium.org/18788002/diff/1/sdk/lib/async/future_impl.dar... sdk/lib/async/future_impl.dart:15: _Completer() : future = new _FutureImpl<T>() { So Future have a Zone too, so why not just use that zone? As it's constructed just like here, they'll follow each other (actually be precisely the same)? https://codereview.chromium.org/18788002/diff/1/sdk/lib/async/future_impl.dar... sdk/lib/async/future_impl.dart:41: if (futureImpl._inSameErrorZone(_zone)) { futureImpl._zone and below? https://codereview.chromium.org/18788002/diff/1/tests/standalone/io/async_cat... File tests/standalone/io/async_catch_errors_test.dart (right): https://codereview.chromium.org/18788002/diff/1/tests/standalone/io/async_cat... tests/standalone/io/async_catch_errors_test.dart:10: Stream catchErrors(void body()) { This is not used. https://codereview.chromium.org/18788002/diff/1/tests/standalone/io/async_cat... tests/standalone/io/async_catch_errors_test.dart:50: var test = tests.last; Heh, this looks a lot like what an iterator can ;) https://codereview.chromium.org/18788002/diff/1/tests/standalone/io/async_cat... tests/standalone/io/async_catch_errors_test.dart:60: }, onError: (err) { Add a counter, to test for max 1. https://codereview.chromium.org/18788002/diff/1/tests/standalone/io/async_cat... tests/standalone/io/async_catch_errors_test.dart:62: events.add("SocketException"); How is events in scope here? https://codereview.chromium.org/18788002/diff/1/tests/standalone/io/async_cat... tests/standalone/io/async_catch_errors_test.dart:70: }, onError: (err) { Ditto as above.
https://codereview.chromium.org/18788002/diff/1/sdk/lib/async/future_impl.dart File sdk/lib/async/future_impl.dart (right): https://codereview.chromium.org/18788002/diff/1/sdk/lib/async/future_impl.dar... sdk/lib/async/future_impl.dart:11: final Future<T> future; On 2013/07/05 17:10:50, Anders Johnsen wrote: > It's okay to type this _FutureImpl, and then avoid the casts in the methods, as > this is not exposed (it's the private _Completer). I think I once had a _FutureImpl here, but Lasse asked me to change it. https://codereview.chromium.org/18788002/diff/1/sdk/lib/async/future_impl.dar... sdk/lib/async/future_impl.dart:15: _Completer() : future = new _FutureImpl<T>() { On 2013/07/05 17:10:50, Anders Johnsen wrote: > So Future have a Zone too, so why not just use that zone? As it's constructed > just like here, they'll follow each other (actually be precisely the same)? Right... No need for Zones in the completer. Removed. https://codereview.chromium.org/18788002/diff/1/sdk/lib/async/future_impl.dar... sdk/lib/async/future_impl.dart:41: if (futureImpl._inSameErrorZone(_zone)) { On 2013/07/05 17:10:50, Anders Johnsen wrote: > futureImpl._zone and below? Actually it must be the same zone now, so I could remove the test.
https://codereview.chromium.org/18788002/diff/1/sdk/lib/async/future_impl.dart File sdk/lib/async/future_impl.dart (right): https://codereview.chromium.org/18788002/diff/1/sdk/lib/async/future_impl.dar... sdk/lib/async/future_impl.dart:11: final Future<T> future; The argument was that is that the field is public, and I want it to match the signature of Completer. It may be irrelevant, since both this class and _FutureImpl are private, so it's not like you can see a difference without mirrors, and with mirrors you can see everything anyway. https://codereview.chromium.org/18788002/diff/1/sdk/lib/async/future_impl.dar... sdk/lib/async/future_impl.dart:15: _Completer() : future = new _FutureImpl<T>() { My interpretation too. Can't you just use future._zone?
https://codereview.chromium.org/18788002/diff/1/sdk/lib/async/future_impl.dart File sdk/lib/async/future_impl.dart (right): https://codereview.chromium.org/18788002/diff/1/sdk/lib/async/future_impl.dar... sdk/lib/async/future_impl.dart:15: _Completer() : future = new _FutureImpl<T>() { On 2013/07/05 18:49:43, Lasse Reichstein Nielsen wrote: > My interpretation too. Can't you just use future._zone? Completer.zone is already removed.
lgtm https://codereview.chromium.org/18788002/diff/1/tests/standalone/io/async_cat... File tests/standalone/io/async_catch_errors_test.dart (right): https://codereview.chromium.org/18788002/diff/1/tests/standalone/io/async_cat... tests/standalone/io/async_catch_errors_test.dart:62: events.add("SocketException"); Declared at top-level in the code below. https://codereview.chromium.org/18788002/diff/5001/sdk/lib/async/future_impl.... File sdk/lib/async/future_impl.dart (right): https://codereview.chromium.org/18788002/diff/5001/sdk/lib/async/future_impl.... sdk/lib/async/future_impl.dart:39: futureImpl._zone.cancelCallbackExpectation(); Is this too early if _setFutureError is async? We don't want the zone to go away before the future is completed. Should the future be the one to decide when it has received its callback, instead of the controller doing it? https://codereview.chromium.org/18788002/diff/5001/tests/standalone/io/async_... File tests/standalone/io/async_catch_errors_test.dart (right): https://codereview.chromium.org/18788002/diff/5001/tests/standalone/io/async_... tests/standalone/io/async_catch_errors_test.dart:10: Stream catchErrors(void body()) { As Anders pointed out: This function is not used. https://codereview.chromium.org/18788002/diff/5001/tests/standalone/io/async_... tests/standalone/io/async_catch_errors_test.dart:32: void startTests() { Could you move the boilerplate code below main, so the tests come first, then the main function that starts it all, and then the helper functions below. It's hard to see what is what in this file. https://codereview.chromium.org/18788002/diff/5001/tests/standalone/io/async_... tests/standalone/io/async_catch_errors_test.dart:41: events); Indentation is off by one. https://codereview.chromium.org/18788002/diff/5001/tests/standalone/io/async_... tests/standalone/io/async_catch_errors_test.dart:50: var test = tests.last; removeLast() ? https://codereview.chromium.org/18788002/diff/5001/tests/standalone/io/async_... tests/standalone/io/async_catch_errors_test.dart:74: }); Could you return a Future that completes when you are done? Then you could chain the tests using .then. https://codereview.chromium.org/18788002/diff/5001/tests/standalone/io/async_... tests/standalone/io/async_catch_errors_test.dart:78: // In reverse order: Consider using a new Queue.from(your list) and use removeFirst, then you don't have to reverse it. Or just use an index into the list that is incremented in launchNextTest. Or just an Iterator, as Anders suggested. The "reverse order" is just distracting.
https://codereview.chromium.org/18788002/diff/1/tests/standalone/io/async_cat... File tests/standalone/io/async_catch_errors_test.dart (right): https://codereview.chromium.org/18788002/diff/1/tests/standalone/io/async_cat... tests/standalone/io/async_catch_errors_test.dart:10: Stream catchErrors(void body()) { On 2013/07/05 17:10:50, Anders Johnsen wrote: > This is not used. Done. https://codereview.chromium.org/18788002/diff/1/tests/standalone/io/async_cat... tests/standalone/io/async_catch_errors_test.dart:50: var test = tests.last; On 2013/07/05 17:10:50, Anders Johnsen wrote: > Heh, this looks a lot like what an iterator can ;) Done. https://codereview.chromium.org/18788002/diff/1/tests/standalone/io/async_cat... tests/standalone/io/async_catch_errors_test.dart:60: }, onError: (err) { On 2013/07/05 17:10:50, Anders Johnsen wrote: > Add a counter, to test for max 1. Done. https://codereview.chromium.org/18788002/diff/1/tests/standalone/io/async_cat... tests/standalone/io/async_catch_errors_test.dart:70: }, onError: (err) { On 2013/07/05 17:10:50, Anders Johnsen wrote: > Ditto as above. Done. https://codereview.chromium.org/18788002/diff/5001/sdk/lib/async/future_impl.... File sdk/lib/async/future_impl.dart (right): https://codereview.chromium.org/18788002/diff/5001/sdk/lib/async/future_impl.... sdk/lib/async/future_impl.dart:39: futureImpl._zone.cancelCallbackExpectation(); On 2013/07/05 19:00:07, Lasse Reichstein Nielsen wrote: > Is this too early if _setFutureError is async? We don't want the zone to go away > before the future is completed. > > Should the future be the one to decide when it has received its callback, > instead of the controller doing it? Moved it down to _setFutureValue and _setFutureError. Ideally it would be the future that decides, but that would require some refactoring. https://codereview.chromium.org/18788002/diff/5001/tests/standalone/io/async_... File tests/standalone/io/async_catch_errors_test.dart (right): https://codereview.chromium.org/18788002/diff/5001/tests/standalone/io/async_... tests/standalone/io/async_catch_errors_test.dart:10: Stream catchErrors(void body()) { On 2013/07/05 19:00:07, Lasse Reichstein Nielsen wrote: > As Anders pointed out: This function is not used. Done. https://codereview.chromium.org/18788002/diff/5001/tests/standalone/io/async_... tests/standalone/io/async_catch_errors_test.dart:32: void startTests() { On 2013/07/05 19:00:07, Lasse Reichstein Nielsen wrote: > Could you move the boilerplate code below main, so the tests come first, then > the main function that starts it all, and then the helper functions below. It's > hard to see what is what in this file. Done. https://codereview.chromium.org/18788002/diff/5001/tests/standalone/io/async_... tests/standalone/io/async_catch_errors_test.dart:41: events); On 2013/07/05 19:00:07, Lasse Reichstein Nielsen wrote: > Indentation is off by one. Done. https://codereview.chromium.org/18788002/diff/5001/tests/standalone/io/async_... tests/standalone/io/async_catch_errors_test.dart:50: var test = tests.last; On 2013/07/05 19:00:07, Lasse Reichstein Nielsen wrote: > removeLast() ? Using iterator now. https://codereview.chromium.org/18788002/diff/5001/tests/standalone/io/async_... tests/standalone/io/async_catch_errors_test.dart:74: }); On 2013/07/05 19:00:07, Lasse Reichstein Nielsen wrote: > Could you return a Future that completes when you are done? > Then you could chain the tests using .then. Done. https://codereview.chromium.org/18788002/diff/5001/tests/standalone/io/async_... tests/standalone/io/async_catch_errors_test.dart:78: // In reverse order: On 2013/07/05 19:00:07, Lasse Reichstein Nielsen wrote: > Consider using a new Queue.from(your list) and use removeFirst, then you don't > have to reverse it. > Or just use an index into the list that is incremented in launchNextTest. Or > just an Iterator, as Anders suggested. > The "reverse order" is just distracting. Changed to iterator and then to futures.
PTAL. I forgot to commit and now I had to merge with Lasse's change in the future_impl.
lgtm, but wait for Lasses comments as well. https://codereview.chromium.org/18788002/diff/15001/tests/standalone/io/async... File tests/standalone/io/async_catch_errors_test.dart (right): https://codereview.chromium.org/18788002/diff/15001/tests/standalone/io/async... tests/standalone/io/async_catch_errors_test.dart:42: testSocketException() Just an idea, but you could remove events and change it to Future.wait([testSocketException(), testFileException()]) .then((events) { ...}); and let the completer complete with the right value in the correct cases only.
https://codereview.chromium.org/18788002/diff/15001/tests/standalone/io/async... File tests/standalone/io/async_catch_errors_test.dart (right): https://codereview.chromium.org/18788002/diff/15001/tests/standalone/io/async... tests/standalone/io/async_catch_errors_test.dart:42: testSocketException() On 2013/07/11 13:43:37, Anders Johnsen wrote: > Just an idea, but you could remove events and change it to > > Future.wait([testSocketException(), > testFileException()]) > .then((events) { ...}); > > and let the completer complete with the right value in the correct cases only. Yes. But frequently I add events at other locations. It allows to trace execution.
lgtm https://codereview.chromium.org/18788002/diff/15001/sdk/lib/async/future_impl... File sdk/lib/async/future_impl.dart (right): https://codereview.chromium.org/18788002/diff/15001/sdk/lib/async/future_impl... sdk/lib/async/future_impl.dart:64: // the expectation without shutting down the zone. Should this be in the _AsyncCompleter above?
https://codereview.chromium.org/18788002/diff/15001/sdk/lib/async/future_impl... File sdk/lib/async/future_impl.dart (right): https://codereview.chromium.org/18788002/diff/15001/sdk/lib/async/future_impl... sdk/lib/async/future_impl.dart:64: // the expectation without shutting down the zone. On 2013/07/12 12:53:57, Lasse Reichstein Nielsen wrote: > Should this be in the _AsyncCompleter above? Done.
Message was sent while issue was closed.
Committed patchset #5 manually as r24947 (presubmit successful). |
