From fff8841349e2e0b73d170086fc35df23eb867d9e Mon Sep 17 00:00:00 2001 From: George Lesica Date: Tue, 4 Apr 2017 20:19:23 -0600 Subject: [PATCH 1/4] RAP-1780 Handle single-subscription stream controllers better. --- lib/src/disposable/disposable.dart | 14 +++++++++++++- test/unit/vm/disposable_test.dart | 25 +++++++++++++++++++++++-- 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/lib/src/disposable/disposable.dart b/lib/src/disposable/disposable.dart index 7ab54378..ce7d045f 100644 --- a/lib/src/disposable/disposable.dart +++ b/lib/src/disposable/disposable.dart @@ -300,8 +300,20 @@ class Disposable implements _Disposable, DisposableManagerV3 { @override void manageStreamController(StreamController controller) { _throwOnInvalidCall('manageStreamController', 'controller', controller); + // If a single-subscription stream has a subscription and that + // subscription is subsequently canceled, the `done` future will + // complete, but there is no other way for us to tell that this + // is what has happened. If we then listen to the stream (since + // closing a stream that was never listen-to throws) we'll get + // an exception. This workaround allows us to "know" when a + // subscription has been canceled so we don't bother trying to + // listen to the stream before closing it. + bool isDone = false; + controller.done.then((_) { + isDone = true; + }); _internalDisposables.add(new _InternalDisposable(() { - if (!controller.hasListener) { + if (!controller.hasListener && !controller.isClosed && !isDone) { controller.stream.listen((_) {}); } return controller.close(); diff --git a/test/unit/vm/disposable_test.dart b/test/unit/vm/disposable_test.dart index 494b3753..d3abb098 100644 --- a/test/unit/vm/disposable_test.dart +++ b/test/unit/vm/disposable_test.dart @@ -262,8 +262,29 @@ void main() { }); test( - 'should close a single-subscription stream with no listener' - 'when parent is disposed', () async { + 'should complete normally for a single-subscription stream with ' + 'a listener that has been closed when parent is disposed', () async { + var controller = new StreamController(); + var sub = controller.stream.listen(expectAsync1((_) {}, count: 0)); + thing.manageStreamController(controller); + await controller.close(); + await thing.dispose(); + await sub.cancel(); + }); + + test( + 'should complete normally for a single-subscription stream with a ' + 'canceled listener when parent is disposed', () async { + var controller = new StreamController(); + var sub = controller.stream.listen(expectAsync1((_) {}, count: 0)); + thing.manageStreamController(controller); + await sub.cancel(); + await thing.dispose(); + }); + + test( + 'should close a single-subscription stream that never had a ' + 'listener when parent is disposed', () async { var controller = new StreamController(); thing.manageStreamController(controller); expect(controller.isClosed, isFalse); From 513379f0c3466d225d3940181d3011a4afb7d244 Mon Sep 17 00:00:00 2001 From: George Lesica Date: Wed, 5 Apr 2017 15:44:52 -0600 Subject: [PATCH 2/4] RAP-1780 Remove internal disposables on stream close. --- lib/src/disposable/disposable.dart | 20 +++++++++++--------- test/unit/vm/disposable_test.dart | 4 ++-- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/lib/src/disposable/disposable.dart b/lib/src/disposable/disposable.dart index ce7d045f..1c84729a 100644 --- a/lib/src/disposable/disposable.dart +++ b/lib/src/disposable/disposable.dart @@ -30,10 +30,10 @@ class _InternalDisposable implements _Disposable { @override Future dispose() { - var disposeFuture = _disposer(); + var disposeFuture = _disposer != null ? _disposer() : null; _disposer = null; if (disposeFuture == null) { - return new Future(() => null); + return new Future.value(); } return disposeFuture.then((_) => null); } @@ -304,20 +304,22 @@ class Disposable implements _Disposable, DisposableManagerV3 { // subscription is subsequently canceled, the `done` future will // complete, but there is no other way for us to tell that this // is what has happened. If we then listen to the stream (since - // closing a stream that was never listen-to throws) we'll get - // an exception. This workaround allows us to "know" when a + // closing a stream that was never listened to never completes) we'll + // get an exception. This workaround allows us to "know" when a // subscription has been canceled so we don't bother trying to // listen to the stream before closing it. bool isDone = false; - controller.done.then((_) { - isDone = true; - }); - _internalDisposables.add(new _InternalDisposable(() { + var disposable = new _InternalDisposable(() { if (!controller.hasListener && !controller.isClosed && !isDone) { controller.stream.listen((_) {}); } return controller.close(); - })); + }); + controller.done.then((_) { + isDone = true; + disposable.dispose(); + }); + _internalDisposables.add(disposable); } @mustCallSuper diff --git a/test/unit/vm/disposable_test.dart b/test/unit/vm/disposable_test.dart index d3abb098..695721dd 100644 --- a/test/unit/vm/disposable_test.dart +++ b/test/unit/vm/disposable_test.dart @@ -262,8 +262,8 @@ void main() { }); test( - 'should complete normally for a single-subscription stream with ' - 'a listener that has been closed when parent is disposed', () async { + 'should allow subscriptions for a single-subscription stream to be ' + 'cancelled after parent has been disposed', () async { var controller = new StreamController(); var sub = controller.stream.listen(expectAsync1((_) {}, count: 0)); thing.manageStreamController(controller); From 3974c9b117ca5c9439f02aecb36f42e18a77fe9d Mon Sep 17 00:00:00 2001 From: George Lesica Date: Wed, 5 Apr 2017 16:13:48 -0600 Subject: [PATCH 3/4] RAP-1780 Remove controller disposable from internal list on close. --- lib/src/disposable/disposable.dart | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/src/disposable/disposable.dart b/lib/src/disposable/disposable.dart index 1c84729a..6918ffe9 100644 --- a/lib/src/disposable/disposable.dart +++ b/lib/src/disposable/disposable.dart @@ -317,6 +317,7 @@ class Disposable implements _Disposable, DisposableManagerV3 { }); controller.done.then((_) { isDone = true; + _internalDisposables.remove(disposable); disposable.dispose(); }); _internalDisposables.add(disposable); From 57679b00a3fe8d01750eef4177230d6ad35ef85d Mon Sep 17 00:00:00 2001 From: George Lesica Date: Wed, 5 Apr 2017 17:25:57 -0600 Subject: [PATCH 4/4] RAP-1780 Re-clarify test description. --- test/unit/vm/disposable_test.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/unit/vm/disposable_test.dart b/test/unit/vm/disposable_test.dart index 695721dd..74153bce 100644 --- a/test/unit/vm/disposable_test.dart +++ b/test/unit/vm/disposable_test.dart @@ -262,8 +262,8 @@ void main() { }); test( - 'should allow subscriptions for a single-subscription stream to be ' - 'cancelled after parent has been disposed', () async { + 'should complete normally for a single-subscription stream, with ' + 'a listener, that has been closed when parent is disposed', () async { var controller = new StreamController(); var sub = controller.stream.listen(expectAsync1((_) {}, count: 0)); thing.manageStreamController(controller);