Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1220963002:
Add an AsyncThunk class. (Closed)
|
Created:
5 years, 5 months ago by nweiz Modified:
5 years, 5 months ago Reviewers:
Lasse Reichstein Nielsen CC:
reviews_dartlang.org Base URL:
git@github.com:dart-lang/async.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdd an AsyncThunk class.
R=lrn@google.com
Committed: https://github.com/dart-lang/async/commit/9ff2614820d95926362005ff511056c67f105032
Patch Set 1 #
Total comments: 2
Patch Set 2 : Code review changes #
Total comments: 10
Patch Set 3 : Code review changes #Patch Set 4 : Code review changes #
Total comments: 16
Patch Set 5 : Code review changes #
Messages
Total messages: 15 (1 generated)
nweiz@google.com changed reviewers: + lrn@google.com
https://codereview.chromium.org/1220963002/diff/1/lib/src/async_thunk.dart File lib/src/async_thunk.dart (right): https://codereview.chromium.org/1220963002/diff/1/lib/src/async_thunk.dart#ne... lib/src/async_thunk.dart:30: class AsyncThunk<T> { This can all just be: class AsyncThunk<T> { Future<T> _future; Future<T> run(fn()) { if (_future == null) _future = new Future<T>.sync(fn); return _future; } } That doesn't really depend on the cached value being a future, so it could just be: class InitOnce { var _value; run(fn()) { // maybe a little more complexity to handle fn returning null. if (_value == null) _value = fn(); return _value; } } that you call with: _initOnce.run(() => new Future<T>.sync(fn)); instead of _closeThunk.run(fn); So, I'm not entirely sure I think this class carries its own weight. Introducing a class means introducing a concept that the user needs to understand and remember. The concept here is so basic that it might be causing more confusion to introduce a new name for it than can be outweighed by the complexity it reduces when using it.
Code review changes
https://codereview.chromium.org/1220963002/diff/1/lib/src/async_thunk.dart File lib/src/async_thunk.dart (right): https://codereview.chromium.org/1220963002/diff/1/lib/src/async_thunk.dart#ne... lib/src/async_thunk.dart:30: class AsyncThunk<T> { On 2015/07/01 05:23:18, Lasse Reichstein Nielsen wrote: > This can all just be: > > class AsyncThunk<T> { > Future<T> _future; > Future<T> run(fn()) { > if (_future == null) _future = new Future<T>.sync(fn); > return _future; > } > } Done. > That doesn't really depend on the cached value being a future, so it could just > be: > class InitOnce { > var _value; > run(fn()) { > // maybe a little more complexity to handle fn returning null. > if (_value == null) _value = fn(); > return _value; > } > } > > that you call with: > _initOnce.run(() => new Future<T>.sync(fn)); > instead of > _closeThunk.run(fn); > > > So, I'm not entirely sure I think this class carries its own weight. Introducing > a class means introducing a concept that the user needs to understand and > remember. The concept here is so basic that it might be causing more confusion > to introduce a new name for it than can be outweighed by the complexity it > reduces when using it. You're right that this class is simple to implement manually, but the fact that it introduces a concept in its own right is part of why I think it's important to have. It provides a single consistent way of doing a common pattern—for users who use this pattern a lot (generally application authors), being able to name it lowers the conceptual burden rather than increasing it. The test package, where I created this class originally, is a great example: it has a bunch of components that need close methods that behave a certain way, and having a clearly visible indicator of that makes the code easier to follow.
https://codereview.chromium.org/1220963002/diff/20001/lib/src/async_thunk.dart File lib/src/async_thunk.dart (right): https://codereview.chromium.org/1220963002/diff/20001/lib/src/async_thunk.dar... lib/src/async_thunk.dart:30: class AsyncThunk<T> { The "Thunk" name doesn't match what I normally think of as a "thunk": A nullary function passed as a paremeter to something else. The "fn" function is a thunk, but this class doesn't really match. No good idea for a better name, "AsyncRunOnce" is not a great name. "AsyncResultCache"? https://codereview.chromium.org/1220963002/diff/20001/lib/src/async_thunk.dar... lib/src/async_thunk.dart:42: Future<T> run(fn()) { Do you actually expect the run function to be called more than once? Is it called with different arguments? If so, it seems unsafe to not run the functions. If not, maybe pass the function to the constructor instead of passing it to each run call (it's slightly misleading to be able to pass different functions with each call if only the first call matters). That would mean you have a delayed future completer! You provide a function to run when you create the DelayedFutureCompleter, but delay running it until later. You can't change your mind about what to complete with. So, signture: class DelayedCompleter<T> { Future<T> get future; DelayedCompleter(T computation()); // Can be run more than once, starts computation the first time it's // run, completes [future] when computation finishes. void start(); // or "run". } (Maybe even add "set computation(T computation())" that can't be used after calling start). I think that would be better as an abstraction than the current class. https://codereview.chromium.org/1220963002/diff/20001/lib/src/future_group.dart File lib/src/future_group.dart (right): https://codereview.chromium.org/1220963002/diff/20001/lib/src/future_group.da... lib/src/future_group.dart:25: int get pending => _pending; Unrelated change?
Code review changes
https://codereview.chromium.org/1220963002/diff/20001/lib/src/async_thunk.dart File lib/src/async_thunk.dart (right): https://codereview.chromium.org/1220963002/diff/20001/lib/src/async_thunk.dar... lib/src/async_thunk.dart:30: class AsyncThunk<T> { On 2015/07/02 12:25:58, Lasse Reichstein Nielsen wrote: > The "Thunk" name doesn't match what I normally think of as a "thunk": A nullary > function passed as a paremeter to something else. > The "fn" function is a thunk, but this class doesn't really match. > > No good idea for a better name, "AsyncRunOnce" is not a great name. > "AsyncResultCache"? The idea was to evoke the lazy-evaluation language use of memoized thunks; this has the same "run once then save the value" behavior. I'm okay with changing it to AsyncResultCache, though. https://codereview.chromium.org/1220963002/diff/20001/lib/src/async_thunk.dar... lib/src/async_thunk.dart:42: Future<T> run(fn()) { On 2015/07/02 12:25:58, Lasse Reichstein Nielsen wrote: > Do you actually expect the run function to be called more than once? > Is it called with different arguments? If so, it seems unsafe to not run the > functions. If not, maybe pass the function to the constructor instead of passing > it to each run call (it's slightly misleading to be able to pass different > functions with each call if only the first call matters). > > That would mean you have a delayed future completer! > > You provide a function to run when you create the DelayedFutureCompleter, but > delay running it until later. You can't change your mind about what to complete > with. So, signture: > > class DelayedCompleter<T> { > Future<T> get future; > DelayedCompleter(T computation()); > // Can be run more than once, starts computation the first time it's > // run, completes [future] when computation finishes. > void start(); // or "run". > } > > (Maybe even add "set computation(T computation())" that can't be used after > calling start). > > I think that would be better as an abstraction than the current class. That would work well with slightly different language semantics, but Dart's restrictions on instance variable initializers make that much uglier to use in practice. I really want the body of the cached method to be in the same place as the declaration, and that's not possible if the body has to be defined when the cache is initialized, since the initialization would have to be done in the constructor. https://codereview.chromium.org/1220963002/diff/20001/lib/src/future_group.dart File lib/src/future_group.dart (right): https://codereview.chromium.org/1220963002/diff/20001/lib/src/future_group.da... lib/src/future_group.dart:25: int get pending => _pending; On 2015/07/02 12:25:58, Lasse Reichstein Nielsen wrote: > Unrelated change? Done.
https://codereview.chromium.org/1220963002/diff/20001/lib/src/async_thunk.dart File lib/src/async_thunk.dart (right): https://codereview.chromium.org/1220963002/diff/20001/lib/src/async_thunk.dar... lib/src/async_thunk.dart:30: class AsyncThunk<T> { I thought so, and it might be possible to put the Thunk into the name, but the class is the memoizer, not the thunk. AsyncThunkMemoizer? https://codereview.chromium.org/1220963002/diff/20001/lib/src/async_thunk.dar... lib/src/async_thunk.dart:42: Future<T> run(fn()) { So you only expect the "run" function to be called from one place. Nothing prevents calling it from different locations with different thunks, but we can at least discourage it by saying that it won't work anyway. I still think it's misleading to call "run(thunk)" and it doesn't actually run. So, how about some slight renamings and modifications? :) class ThunkMemoizer<T> { Future<T> _result; /// Call to return the result of calling [thunk]. /// /// Only the first call to [runOnce] does anything, /// further calls return the same result as the first call. /// Calling with a different [thunk] on later calls will not /// make any difference, the intended use is to only call the /// the `runOnce` function from one location and always with the /// same (or equivalent) function as argument. Future<T> runOnce(T thunk()) { if (_result == null) _result = new Future<T>.sync(thunk); return _result; } } With a little luck the runOnce will be inlined at the call point, and the thunk won't even be created on the second run. By making the call return the Future instead of having it on the side, you avoid having to consider whether you can read [future] before calling [run]. It seems like a slightly more general design.
Code review changes
https://codereview.chromium.org/1220963002/diff/20001/lib/src/async_thunk.dart File lib/src/async_thunk.dart (right): https://codereview.chromium.org/1220963002/diff/20001/lib/src/async_thunk.dar... lib/src/async_thunk.dart:30: class AsyncThunk<T> { On 2015/07/07 09:32:40, Lasse Reichstein Nielsen wrote: > I thought so, and it might be possible to put the Thunk into the name, but the > class is the memoizer, not the thunk. > AsyncThunkMemoizer? Changed to AsyncMemoizer. https://codereview.chromium.org/1220963002/diff/20001/lib/src/async_thunk.dar... lib/src/async_thunk.dart:42: Future<T> run(fn()) { On 2015/07/07 09:32:40, Lasse Reichstein Nielsen wrote: > So you only expect the "run" function to be called from one place. Nothing > prevents calling it from different locations with different thunks, but we can > at least discourage it by saying that it won't work anyway. > > I still think it's misleading to call "run(thunk)" and it doesn't actually run. > > So, how about some slight renamings and modifications? :) > > class ThunkMemoizer<T> { > Future<T> _result; > /// Call to return the result of calling [thunk]. > /// > /// Only the first call to [runOnce] does anything, > /// further calls return the same result as the first call. > /// Calling with a different [thunk] on later calls will not > /// make any difference, the intended use is to only call the > /// the `runOnce` function from one location and always with the > /// same (or equivalent) function as argument. > Future<T> runOnce(T thunk()) { > if (_result == null) _result = new Future<T>.sync(thunk); > return _result; > } > } > > With a little luck the runOnce will be inlined at the call point, and the thunk > won't even be created on the second run. > > By making the call return the Future instead of having it on the side, you avoid > having to consider whether you can read [future] before calling [run]. > > It seems like a slightly more general design. Done.
LGTM. I like it now :) https://codereview.chromium.org/1220963002/diff/60001/CHANGELOG.md File CHANGELOG.md (right): https://codereview.chromium.org/1220963002/diff/60001/CHANGELOG.md#newcode20 CHANGELOG.md:20: - Add an `AsyncResultCache` class for running an asynchronous block of code Rename here too. https://codereview.chromium.org/1220963002/diff/60001/lib/src/async_memoizer.... File lib/src/async_memoizer.dart (right): https://codereview.chromium.org/1220963002/diff/60001/lib/src/async_memoizer.... lib/src/async_memoizer.dart:9: /// A class for running an asynchronous method body exactly once and caching its method body -> function https://codereview.chromium.org/1220963002/diff/60001/lib/src/async_memoizer.... lib/src/async_memoizer.dart:12: /// This should be stored as an instance variable, and [runOnce] should be Drop the usage of "this", or at least don't make it "should". You can also use it as a local variable and use it from, say, a callback function on a stream. I don't understand the rest of the sentence. What is consistent is that you create one memoizer, then call it from some code that will run more than once. How about: An `AsyncMemoizer` is used when some function may be run multiple times in order to get its result, but it only actually needs to be run once for its effect. To memoize the result of an async function, you can create a memoizer outside the function (for example as an instance field if you want to memoize the result of a method), and then wrap the function's body in a call to [runOnce]. Example: ```dart ... your example ... ``` https://codereview.chromium.org/1220963002/diff/60001/lib/src/async_memoizer.... lib/src/async_memoizer.dart:20: /// ```dart Does DartDoc understand "```dart" now! If so, yey! I'm so tired of indenting by four! https://codereview.chromium.org/1220963002/diff/60001/lib/src/async_memoizer.... lib/src/async_memoizer.dart:38: /// Runs the method body, [fn], if it hasn't been run before. method body -> function. https://codereview.chromium.org/1220963002/diff/60001/lib/src/async_memoizer.... lib/src/async_memoizer.dart:41: Future<T> runOnce(fn()) { Rename "fn" to "computation". This matches the argument to Future.sync and other places where we don't have a better name for an async function. https://codereview.chromium.org/1220963002/diff/60001/test/async_memoizer_tes... File test/async_memoizer_test.dart (right): https://codereview.chromium.org/1220963002/diff/60001/test/async_memoizer_tes... test/async_memoizer_test.dart:17: expect(count, equals(1)); Remember the result too, and check that it contains the same result in both cases. var count = 0; var result = await cache.runOnce(() => count++); expect(count, equals(1)); expect(result, equals(0)); (and ditto below). https://codereview.chromium.org/1220963002/diff/60001/test/async_memoizer_tes... test/async_memoizer_test.dart:31: }); Try with a function returning a Future too, preferably both with a value future and an error future.
Code review changes
https://codereview.chromium.org/1220963002/diff/60001/CHANGELOG.md File CHANGELOG.md (right): https://codereview.chromium.org/1220963002/diff/60001/CHANGELOG.md#newcode20 CHANGELOG.md:20: - Add an `AsyncResultCache` class for running an asynchronous block of code On 2015/07/08 07:37:34, Lasse Reichstein Nielsen wrote: > Rename here too. Done. https://codereview.chromium.org/1220963002/diff/60001/lib/src/async_memoizer.... File lib/src/async_memoizer.dart (right): https://codereview.chromium.org/1220963002/diff/60001/lib/src/async_memoizer.... lib/src/async_memoizer.dart:9: /// A class for running an asynchronous method body exactly once and caching its On 2015/07/08 07:37:34, Lasse Reichstein Nielsen wrote: > method body -> function Done. https://codereview.chromium.org/1220963002/diff/60001/lib/src/async_memoizer.... lib/src/async_memoizer.dart:12: /// This should be stored as an instance variable, and [runOnce] should be On 2015/07/08 07:37:34, Lasse Reichstein Nielsen wrote: > Drop the usage of "this", or at least don't make it "should". You can also use > it as a local variable and use it from, say, a callback function on a stream. > I don't understand the rest of the sentence. > > What is consistent is that you create one memoizer, then call it from some code > that will run more than once. > > How about: > > An `AsyncMemoizer` is used when some function may be run multiple times in order > to get its result, but it only actually needs to be run once for its effect. > To memoize the result of an async function, you can create a memoizer outside > the function (for example as an instance field if you want to memoize the result > of a method), and then wrap the function's body in a call to [runOnce]. > > Example: > ```dart > ... your example ... > ``` > > Done. https://codereview.chromium.org/1220963002/diff/60001/lib/src/async_memoizer.... lib/src/async_memoizer.dart:20: /// ```dart On 2015/07/08 07:37:34, Lasse Reichstein Nielsen wrote: > Does DartDoc understand "```dart" now! If so, yey! I'm so tired of indenting by > four! I think so? I'm not 100% sure, but I'll keep an eye on this and change it if it breaks. https://codereview.chromium.org/1220963002/diff/60001/lib/src/async_memoizer.... lib/src/async_memoizer.dart:38: /// Runs the method body, [fn], if it hasn't been run before. On 2015/07/08 07:37:34, Lasse Reichstein Nielsen wrote: > method body -> function. Done. https://codereview.chromium.org/1220963002/diff/60001/lib/src/async_memoizer.... lib/src/async_memoizer.dart:41: Future<T> runOnce(fn()) { On 2015/07/08 07:37:34, Lasse Reichstein Nielsen wrote: > Rename "fn" to "computation". > This matches the argument to Future.sync and other places where we don't have a > better name for an async function. Done. https://codereview.chromium.org/1220963002/diff/60001/test/async_memoizer_tes... File test/async_memoizer_test.dart (right): https://codereview.chromium.org/1220963002/diff/60001/test/async_memoizer_tes... test/async_memoizer_test.dart:17: expect(count, equals(1)); On 2015/07/08 07:37:34, Lasse Reichstein Nielsen wrote: > Remember the result too, and check that it contains the same result in both > cases. > > var count = 0; > var result = await cache.runOnce(() => count++); > expect(count, equals(1)); > expect(result, equals(0)); > > (and ditto below). Done. https://codereview.chromium.org/1220963002/diff/60001/test/async_memoizer_tes... test/async_memoizer_test.dart:31: }); On 2015/07/08 07:37:34, Lasse Reichstein Nielsen wrote: > Try with a function returning a Future too, preferably both with a value future > and an error future. Done.
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as 9ff2614820d95926362005ff511056c67f105032 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
