From a5a99a34e6b07bfca80afa0c577233ff976bd13b Mon Sep 17 00:00:00 2001 From: Nikolas Rimikis Date: Thu, 7 Sep 2023 00:30:44 +0200 Subject: [PATCH] fix(neon): remove and dispose dagnling blocs after logout Signed-off-by: Nikolas Rimikis --- packages/neon/neon/lib/src/bloc/bloc.dart | 18 +----- .../neon/neon/lib/src/blocs/accounts.dart | 53 ++++++++++------- .../neon/lib/src/models/account_cache.dart | 41 +++++++++++++ .../lib/src/models/app_implementation.dart | 7 ++- .../neon/neon/lib/src/models/disposable.dart | 33 +++++++++++ .../neon/lib/src/settings/models/option.dart | 3 +- .../settings/models/options_collection.dart | 8 +-- .../neon/neon/test/account_cache_test.dart | 59 +++++++++++++++++++ packages/neon/neon/test/disposable_test.dart | 39 ++++++++++++ 9 files changed, 217 insertions(+), 44 deletions(-) create mode 100644 packages/neon/neon/lib/src/models/account_cache.dart create mode 100644 packages/neon/neon/lib/src/models/disposable.dart create mode 100644 packages/neon/neon/test/account_cache_test.dart create mode 100644 packages/neon/neon/test/disposable_test.dart diff --git a/packages/neon/neon/lib/src/bloc/bloc.dart b/packages/neon/neon/lib/src/bloc/bloc.dart index 72629c47..be6582f5 100644 --- a/packages/neon/neon/lib/src/bloc/bloc.dart +++ b/packages/neon/neon/lib/src/bloc/bloc.dart @@ -1,9 +1,11 @@ import 'dart:async'; import 'package:flutter/foundation.dart'; +import 'package:neon/src/models/disposable.dart'; import 'package:neon/src/utils/request_manager.dart'; -abstract class Bloc { +abstract class Bloc implements Disposable { + @override @mustCallSuper void dispose(); } @@ -44,17 +46,3 @@ abstract class InteractiveBloc extends Bloc { } } } - -extension DisposeableIterableBloc on Iterable { - void disposeAll() { - for (final bloc in this) { - bloc.dispose(); - } - } -} - -extension DisposeableMapBloc on Map { - void disposeAll() { - values.disposeAll(); - } -} diff --git a/packages/neon/neon/lib/src/blocs/accounts.dart b/packages/neon/neon/lib/src/blocs/accounts.dart index c4ca84a8..ae40f0be 100644 --- a/packages/neon/neon/lib/src/blocs/accounts.dart +++ b/packages/neon/neon/lib/src/blocs/accounts.dart @@ -1,6 +1,7 @@ import 'dart:async'; import 'dart:convert'; +import 'package:collection/collection.dart'; import 'package:flutter/foundation.dart'; import 'package:meta/meta.dart'; import 'package:neon/src/bloc/bloc.dart'; @@ -10,6 +11,7 @@ import 'package:neon/src/blocs/unified_search.dart'; import 'package:neon/src/blocs/user_details.dart'; import 'package:neon/src/blocs/user_statuses.dart'; import 'package:neon/src/models/account.dart'; +import 'package:neon/src/models/account_cache.dart'; import 'package:neon/src/models/app_implementation.dart'; import 'package:neon/src/settings/models/storage.dart'; import 'package:neon/src/utils/account_options.dart'; @@ -97,30 +99,40 @@ class AccountsBloc extends Bloc implements AccountsBlocEvents, AccountsBlocState setActiveAccount(as.first); } } + + accounts.listen((final accounts) { + _accountsOptions.pruneAgainst(accounts); + _appsBlocs.pruneAgainst(accounts); + _capabilitiesBlocs.pruneAgainst(accounts); + _userDetailsBlocs.pruneAgainst(accounts); + _userStatusesBlocs.pruneAgainst(accounts); + _unifiedSearchBlocs.pruneAgainst(accounts); + for (final app in _allAppImplementations) { + app.blocsCache.pruneAgainst(accounts); + } + }); } final GlobalOptions _globalOptions; final Iterable _allAppImplementations; - final _accountsOptions = {}; - final _appsBlocs = {}; - final _capabilitiesBlocs = {}; - final _userDetailsBlocs = {}; - final _userStatusesBlocs = {}; - final _unifiedSearchBlocs = {}; + final _accountsOptions = AccountCache(); + final _appsBlocs = AccountCache(); + final _capabilitiesBlocs = AccountCache(); + final _userDetailsBlocs = AccountCache(); + final _userStatusesBlocs = AccountCache(); + final _unifiedSearchBlocs = AccountCache(); @override void dispose() { unawaited(activeAccount.close()); unawaited(accounts.close()); - _appsBlocs.disposeAll(); - _capabilitiesBlocs.disposeAll(); - _userDetailsBlocs.disposeAll(); - _userStatusesBlocs.disposeAll(); - _unifiedSearchBlocs.disposeAll(); - for (final options in _accountsOptions.values) { - options.dispose(); - } + _appsBlocs.dispose(); + _capabilitiesBlocs.dispose(); + _userDetailsBlocs.dispose(); + _userStatusesBlocs.dispose(); + _unifiedSearchBlocs.dispose(); + _accountsOptions.dispose(); } @override @@ -212,8 +224,7 @@ class AccountsBloc extends Bloc implements AccountsBlocEvents, AccountsBlocState /// The options for the specified [account]. /// /// Use [activeOptions] to get them for the [activeAccount]. - AccountSpecificOptions getOptionsFor(final Account account) => - _accountsOptions[account.id] ??= AccountSpecificOptions( + AccountSpecificOptions getOptionsFor(final Account account) => _accountsOptions[account] ??= AccountSpecificOptions( AppStorage(StorageKeys.accounts, account.id), getAppsBlocFor(account), ); @@ -226,7 +237,7 @@ class AccountsBloc extends Bloc implements AccountsBlocEvents, AccountsBlocState /// The appsBloc for the specified [account]. /// /// Use [activeAppsBloc] to get them for the [activeAccount]. - AppsBloc getAppsBlocFor(final Account account) => _appsBlocs[account.id] ??= AppsBloc( + AppsBloc getAppsBlocFor(final Account account) => _appsBlocs[account] ??= AppsBloc( getCapabilitiesBlocFor(account), this, account, @@ -242,7 +253,7 @@ class AccountsBloc extends Bloc implements AccountsBlocEvents, AccountsBlocState /// /// Use [activeCapabilitiesBloc] to get them for the [activeAccount]. CapabilitiesBloc getCapabilitiesBlocFor(final Account account) => - _capabilitiesBlocs[account.id] ??= CapabilitiesBloc(account); + _capabilitiesBlocs[account] ??= CapabilitiesBloc(account); /// The userDetailsBloc for the [activeAccount]. /// @@ -253,7 +264,7 @@ class AccountsBloc extends Bloc implements AccountsBlocEvents, AccountsBlocState /// /// Use [activeUerDetailsBloc] to get them for the [activeAccount]. UserDetailsBloc getUserDetailsBlocFor(final Account account) => - _userDetailsBlocs[account.id] ??= UserDetailsBloc(account); + _userDetailsBlocs[account] ??= UserDetailsBloc(account); /// The userStatusBloc for the [activeAccount]. /// @@ -264,7 +275,7 @@ class AccountsBloc extends Bloc implements AccountsBlocEvents, AccountsBlocState /// /// Use [activeUserStatusesBloc] to get them for the [activeAccount]. UserStatusesBloc getUserStatusesBlocFor(final Account account) => - _userStatusesBlocs[account.id] ??= UserStatusesBloc(account); + _userStatusesBlocs[account] ??= UserStatusesBloc(account); /// The UnifiedSearchBloc for the [activeAccount]. /// @@ -275,7 +286,7 @@ class AccountsBloc extends Bloc implements AccountsBlocEvents, AccountsBlocState /// /// Use [activeUnifiedSearchBloc] to get them for the [activeAccount]. UnifiedSearchBloc getUnifiedSearchBlocFor(final Account account) => - _unifiedSearchBlocs[account.id] ??= UnifiedSearchBloc( + _unifiedSearchBlocs[account] ??= UnifiedSearchBloc( getAppsBlocFor(account), account, ); diff --git a/packages/neon/neon/lib/src/models/account_cache.dart b/packages/neon/neon/lib/src/models/account_cache.dart new file mode 100644 index 00000000..0a759e62 --- /dev/null +++ b/packages/neon/neon/lib/src/models/account_cache.dart @@ -0,0 +1,41 @@ +import 'package:neon/models.dart'; +import 'package:neon/src/models/disposable.dart'; + +/// Cache for [Account] specific [Disposable] objects. +class AccountCache implements Disposable { + AccountCache(); + + final Map _cache = {}; + + /// Disposes all entries and clears the cache. + @override + void dispose() { + _cache + ..disposeAll() + ..clear(); + } + + /// Updates the cache against the given [accounts]. + /// + /// Every cache entry with an account no longer in [accounts] is removed and + /// disposed. This method is in O(N²). + void pruneAgainst(final Iterable accounts) { + _cache.removeWhere((final key, final value) { + if (accounts.tryFind(key) == null) { + value.dispose(); + return true; + } + + return false; + }); + } + + /// The value for the given [account], or `null` if [account] is not in the cache. + T? operator [](final Account account) => _cache[account.id]; + + /// Associates the [account] with the given [value]. + /// + /// If the account was already in the cache, its associated value is changed. + /// Otherwise the account/value pair is added to the cache. + void operator []=(final Account account, final T value) => _cache[account.id] = value; +} diff --git a/packages/neon/neon/lib/src/models/app_implementation.dart b/packages/neon/neon/lib/src/models/app_implementation.dart index a36936cd..811884aa 100644 --- a/packages/neon/neon/lib/src/models/app_implementation.dart +++ b/packages/neon/neon/lib/src/models/app_implementation.dart @@ -6,6 +6,7 @@ import 'package:neon/l10n/localizations.dart'; import 'package:neon/src/bloc/bloc.dart'; import 'package:neon/src/blocs/accounts.dart'; import 'package:neon/src/models/account.dart'; +import 'package:neon/src/models/account_cache.dart'; import 'package:neon/src/settings/models/options_collection.dart'; import 'package:neon/src/settings/models/storage.dart'; import 'package:neon/src/widgets/drawer_destination.dart'; @@ -30,9 +31,9 @@ abstract class AppImplementation @mustBeOverridden R get options; - final Map _blocs = {}; + final blocsCache = AccountCache(); - T getBloc(final Account account) => _blocs[account.id] ??= buildBloc(account); + T getBloc(final Account account) => blocsCache[account] ??= buildBloc(account); @protected T buildBloc(final Account account); @@ -113,7 +114,7 @@ abstract class AppImplementation @mustCallSuper void dispose() { options.dispose(); - _blocs.disposeAll(); + blocsCache.dispose(); } /// A custom theme that will be injected into the widget tree. diff --git a/packages/neon/neon/lib/src/models/disposable.dart b/packages/neon/neon/lib/src/models/disposable.dart new file mode 100644 index 00000000..72ada454 --- /dev/null +++ b/packages/neon/neon/lib/src/models/disposable.dart @@ -0,0 +1,33 @@ +import 'package:meta/meta.dart'; + +/// Interface of a disposable class. +abstract interface class Disposable { + /// Discards any resources used by the object. After this is called, the + /// object is not in a usable state and should be discarded (calls to + /// streams or other state should throw after the object is disposed). + /// + /// This method should only be called by the object's owner. + @mustCallSuper + void dispose(); +} + +extension DisposeableIterableBloc on Iterable { + /// Calls [Disposable.dispose] on all entries. + /// + /// The disposed values will not be removed from the iteraable. + void disposeAll() { + for (final bloc in this) { + bloc.dispose(); + } + } +} + +extension DisposeableMapBloc on Map { + /// Calls [Disposable.dispose] on all entries. + /// + /// The disposed values will not be removed from the map. + /// Call [clear] to remove them. + void disposeAll() { + values.disposeAll(); + } +} diff --git a/packages/neon/neon/lib/src/settings/models/option.dart b/packages/neon/neon/lib/src/settings/models/option.dart index 53f05231..af78a9d0 100644 --- a/packages/neon/neon/lib/src/settings/models/option.dart +++ b/packages/neon/neon/lib/src/settings/models/option.dart @@ -3,6 +3,7 @@ import 'dart:async'; import 'package:collection/collection.dart'; import 'package:flutter/foundation.dart'; import 'package:meta/meta.dart'; +import 'package:neon/src/models/disposable.dart'; import 'package:neon/src/models/label_builder.dart'; import 'package:neon/src/settings/models/options_category.dart'; import 'package:neon/src/settings/models/storage.dart'; @@ -13,7 +14,7 @@ import 'package:rxdart/rxdart.dart'; /// See: /// * [ToggleOption] for an Option /// * [SelectOption] for an Option with multiple values -sealed class Option extends ChangeNotifier implements ValueListenable { +sealed class Option extends ChangeNotifier implements ValueListenable, Disposable { /// Creates an Option Option({ required this.storage, diff --git a/packages/neon/neon/lib/src/settings/models/options_collection.dart b/packages/neon/neon/lib/src/settings/models/options_collection.dart index a2e5c310..f62c4fdb 100644 --- a/packages/neon/neon/lib/src/settings/models/options_collection.dart +++ b/packages/neon/neon/lib/src/settings/models/options_collection.dart @@ -1,11 +1,12 @@ import 'package:meta/meta.dart'; +import 'package:neon/src/models/disposable.dart'; import 'package:neon/src/settings/models/exportable.dart'; import 'package:neon/src/settings/models/option.dart'; import 'package:neon/src/settings/models/options_category.dart'; import 'package:neon/src/settings/models/storage.dart'; /// Collection of [Option]s. -abstract class OptionsCollection implements Exportable { +abstract class OptionsCollection implements Exportable, Disposable { OptionsCollection(this.storage); /// Storage backend to use. @@ -30,10 +31,9 @@ abstract class OptionsCollection implements Exportable { /// /// Implementers extending this must call super. @mustCallSuper + @override void dispose() { - for (final option in options) { - option.dispose(); - } + options.disposeAll(); } @override diff --git a/packages/neon/neon/test/account_cache_test.dart b/packages/neon/neon/test/account_cache_test.dart new file mode 100644 index 00000000..26dc80ee --- /dev/null +++ b/packages/neon/neon/test/account_cache_test.dart @@ -0,0 +1,59 @@ +import 'package:mocktail/mocktail.dart'; +import 'package:neon/models.dart'; +import 'package:neon/src/models/account_cache.dart'; +import 'package:neon/src/models/disposable.dart'; +import 'package:test/test.dart'; + +class DisposableMock extends Mock implements Disposable {} + +// ignore: avoid_implementing_value_types +class AccountMock extends Mock implements Account {} + +void main() { + final disposable0 = DisposableMock(); + final disposable1 = DisposableMock(); + final account0 = AccountMock(); + final account1 = AccountMock(); + + when(() => account0.id).thenReturn('key0'); + when(() => account1.id).thenReturn('key1'); + + group('AccountCache', () { + test('map functionality', () { + final cache = AccountCache(); + + expect(cache[account0], isNull); + + cache[account0] = disposable0; + expect(cache[account0], disposable0); + + expect(cache[account0] ??= disposable1, disposable0); + expect(cache[account1] ??= disposable1, disposable1); + }); + + test('prune', () { + final cache = AccountCache(); + cache[account0] = disposable0; + cache[account1] = disposable1; + + cache.pruneAgainst([account0]); + + expect(cache[account0], disposable0); + expect(cache[account1], isNull); + verify(disposable1.dispose).called(1); + }); + + test('dispose', () { + final cache = AccountCache(); + cache[account0] = disposable0; + cache[account1] = disposable1; + + cache.dispose(); + + expect(cache[account0], isNull); + expect(cache[account1], isNull); + verify(disposable0.dispose).called(1); + verify(disposable1.dispose).called(1); + }); + }); +} diff --git a/packages/neon/neon/test/disposable_test.dart b/packages/neon/neon/test/disposable_test.dart new file mode 100644 index 00000000..66e53129 --- /dev/null +++ b/packages/neon/neon/test/disposable_test.dart @@ -0,0 +1,39 @@ +// ignore_for_file: cascade_invocations + +import 'package:mocktail/mocktail.dart'; +import 'package:neon/src/models/disposable.dart'; +import 'package:test/test.dart'; + +class DisposableMock extends Mock implements Disposable {} + +void main() { + test('Disposable extensions', () { + final disposable0 = DisposableMock(); + final disposable1 = DisposableMock(); + final disposable3 = DisposableMock(); + + final list = [ + disposable0, + disposable1, + disposable3, + ]; + + list.disposeAll(); + + verify(disposable0.dispose).called(1); + verify(disposable1.dispose).called(1); + verify(disposable3.dispose).called(1); + + final map = { + 'disposable0': disposable0, + 'disposable1': disposable1, + 'disposable3': disposable3, + }; + + map.disposeAll(); + + verify(disposable0.dispose).called(1); + verify(disposable1.dispose).called(1); + verify(disposable3.dispose).called(1); + }); +}