-
Notifications
You must be signed in to change notification settings - Fork 34
Add FutureGroup.addCancelable()
#726
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
base: main
Are you sure you want to change the base?
Conversation
Package publishing
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
PR HealthLicense Headers ✔️
All source files should start with a license header. This check can be disabled by tagging the PR with API leaks ✔️The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
This check can be disabled by tagging the PR with Breaking changes ✔️
This check can be disabled by tagging the PR with Coverage ✔️
This check for test coverage is informational (issues shown here will not fail the PR). This check can be disabled by tagging the PR with Changelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. This check can be disabled by tagging the PR with |
|
@nex3 is it still your intention to land this PR? Just a friendly ping. |
|
I'd be happy to land it. I'm still waiting on a review. |
|
Then redirecting the friendly ping to @lrhn or @natebosch :) |
pkgs/async/lib/src/future_group.dart
Outdated
| @override | ||
| void add(Future<T> task) { | ||
| void add(Future<T> task) => | ||
| addCancelable(CancelableOperation.fromFuture(task)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I worry about the performance penalty here. By wrapping in a CancelableOperation we'd be creating 3 extra Completer instances along with a few book-keeping objects and function literals and tear-offs, for every Future added to the group.
Would it make sense to build a CancelableOperationGroup instead which has this overhead for the use cases that need it? It may be feasible to build as an adapter on FutureGroup to keep the implementation small.
Would it need to be much more complex than this?
class CancelableOperationGroup<T> {
final FutureGroup<Object?> _group = FutureGroup();
Future<List<T>> get future async =>
(await _group.future).where((e) => e != _sentinel).whereType<T>().toList();
void add(CancelableOperation<T> task) {
final normalizedToSentinel =
task.then((r) => r, onCancel: () => _sentinel);
_group.add(normalizedToSentinel.valueOrCancellation);
}
// other members of FutureGroup may be useful to forward as well
// bool get isClosed => _group.isClosed;
// etc
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that you mention it, we can actually use the same technique in this class to avoid adding overhead for normal futures.
6bc6363 to
32cd9b6
Compare
No description provided.