From 2581bbe203b182fd9839bd2e7ca2ee914c28f51d Mon Sep 17 00:00:00 2001 From: "Alexander \"PapaTutuWawa" Date: Sat, 7 Jan 2023 18:11:41 +0100 Subject: [PATCH] feat: Document the RosterStateManager better --- packages/moxxmpp/lib/src/roster/state.dart | 152 ++++++++++++------- packages/moxxmpp/test/roster_state_test.dart | 22 +-- 2 files changed, 108 insertions(+), 66 deletions(-) diff --git a/packages/moxxmpp/lib/src/roster/state.dart b/packages/moxxmpp/lib/src/roster/state.dart index dbbee3e..cbc81f7 100644 --- a/packages/moxxmpp/lib/src/roster/state.dart +++ b/packages/moxxmpp/lib/src/roster/state.dart @@ -1,6 +1,7 @@ import 'package:meta/meta.dart'; import 'package:moxxmpp/src/events.dart'; import 'package:moxxmpp/src/roster/roster.dart'; +import 'package:synchronized/synchronized.dart'; class _RosterProcessTriple { const _RosterProcessTriple(this.removed, this.modified, this.added); @@ -15,33 +16,65 @@ class RosterCacheLoadResult { final List roster; } +/// This class manages the roster state in order to correctly process and persist +/// roster pushes and facilitate roster versioning requests. abstract class BaseRosterStateManager { - List? currentRoster; - String? currentVersion; + /// The cached version of the roster. If null, then it has not been loaded yet. + List? _currentRoster; + /// The cached version of the roster version. + String? _currentVersion; + + /// A critical section locking both _currentRoster and _currentVersion. + final Lock _lock = Lock(); + + /// Overrideable function + /// Loads the old cached version of the roster and optionally that roster version + /// from persistent storage into a RosterCacheLoadResult object. Future loadRosterCache(); + /// Overrideable function + /// Commits the roster data to persistent storage. + /// + /// [version] is the roster version string. If none was provided, then this value + /// is null. + /// + /// [removed] is a (possibly empty) list of bare JIDs that are removed from the + /// roster. + /// + /// [modified] is a (possibly empty) list of XmppRosterItems that are modified. Correlation with + /// the cache is done using its jid attribute. + /// + /// [added] is a (possibly empty) list of XmppRosterItems that are added by the + /// roster push or roster fetch request. Future commitRoster(String? version, List removed, List modified, List added); + /// Load and cache or return the cached roster version. Future getRosterVersion() async { - await _loadRosterCache(); + return _lock.synchronized(() async { + await _loadRosterCache(); - return currentVersion; + return _currentVersion; + }); } - + + /// Loads the cached roster data into memory, if that has not already happened. + /// NOTE: Must be called from within the _lock critical section. Future _loadRosterCache() async { - if (currentRoster == null) { + if (_currentRoster == null) { final result = await loadRosterCache(); - currentRoster = result.roster; - currentVersion = result.version; + _currentRoster = result.roster; + _currentVersion = result.version; } } - + + /// Processes only single XmppRosterItem [item]. + /// NOTE: Requires to be called from within the _lock critical section. _RosterProcessTriple _handleRosterItem(XmppRosterItem item) { if (item.subscription == 'remove') { // The item has been removed - currentRoster!.removeWhere((i) => i.jid == item.jid); + _currentRoster!.removeWhere((i) => i.jid == item.jid); return _RosterProcessTriple( item.jid, null, @@ -49,10 +82,10 @@ abstract class BaseRosterStateManager { ); } - final index = currentRoster!.indexWhere((i) => i.jid == item.jid); + final index = _currentRoster!.indexWhere((i) => i.jid == item.jid); if (index == -1) { // The item does not exist - currentRoster!.add(item); + _currentRoster!.add(item); return _RosterProcessTriple( null, null, @@ -60,7 +93,7 @@ abstract class BaseRosterStateManager { ); } else { // The item is updated - currentRoster![index] = item; + _currentRoster![index] = item; return _RosterProcessTriple( null, item, @@ -69,59 +102,68 @@ abstract class BaseRosterStateManager { } } + /// Handles a roster push from the RosterManager. Future handleRosterPush(RosterPushEvent event) async { - await _loadRosterCache(); + await _lock.synchronized(() async { + await _loadRosterCache(); - currentVersion = event.ver; - final result = _handleRosterItem(event.item); + _currentVersion = event.ver; + final result = _handleRosterItem(event.item); - if (result.removed != null) { - return commitRoster( - currentVersion, - [result.removed!], - [], - [], - ); - } else if (result.modified != null) { - return commitRoster( - currentVersion, - [], - [result.modified!], - [], - ); - } else if (result.added != null) { - return commitRoster( - currentVersion, - [], - [], - [result.added!], - ); - } + if (result.removed != null) { + return commitRoster( + _currentVersion, + [result.removed!], + [], + [], + ); + } else if (result.modified != null) { + return commitRoster( + _currentVersion, + [], + [result.modified!], + [], + ); + } else if (result.added != null) { + return commitRoster( + _currentVersion, + [], + [], + [result.added!], + ); + } + }); } + /// Handles the result from a roster fetch. Future handleRosterFetch(RosterRequestResult result) async { - final removed = List.empty(growable: true); - final modified = List.empty(growable: true); - final added = List.empty(growable: true); + await _lock.synchronized(() async { + final removed = List.empty(growable: true); + final modified = List.empty(growable: true); + final added = List.empty(growable: true); - await _loadRosterCache(); + await _loadRosterCache(); - currentVersion = result.ver; - for (final item in result.items) { - final result = _handleRosterItem(item); + _currentVersion = result.ver; + for (final item in result.items) { + final result = _handleRosterItem(item); - if (result.removed != null) removed.add(result.removed!); - if (result.modified != null) modified.add(result.modified!); - if (result.added != null) added.add(result.added!); - } + if (result.removed != null) removed.add(result.removed!); + if (result.modified != null) modified.add(result.modified!); + if (result.added != null) added.add(result.added!); + } - await commitRoster( - currentVersion, - removed, - modified, - added, - ); + await commitRoster( + _currentVersion, + removed, + modified, + added, + ); + }); } + + @visibleForTesting + List getRosterItems() => _currentRoster!; } @visibleForTesting diff --git a/packages/moxxmpp/test/roster_state_test.dart b/packages/moxxmpp/test/roster_state_test.dart index 6fd5ebe..94651af 100644 --- a/packages/moxxmpp/test/roster_state_test.dart +++ b/packages/moxxmpp/test/roster_state_test.dart @@ -15,11 +15,11 @@ void main() { ); expect( - rs.currentRoster!.indexWhere((item) => item.jid == 'testuser@server.example') != -1, + rs.getRosterItems().indexWhere((item) => item.jid == 'testuser@server.example') != -1, true, ); expect(rs.loadCount, 1); - expect(rs.currentRoster!.length, 1); + expect(rs.getRosterItems().length, 1); // Receive another roster push await rs.handleRosterPush( @@ -32,11 +32,11 @@ void main() { ); expect( - rs.currentRoster!.indexWhere((item) => item.jid == 'testuser2@server2.example') != -1, + rs.getRosterItems().indexWhere((item) => item.jid == 'testuser2@server2.example') != -1, true, ); expect(rs.loadCount, 1); - expect(rs.currentRoster!.length, 2); + expect(rs.getRosterItems().length, 2); // Remove one of the items await rs.handleRosterPush( @@ -49,15 +49,15 @@ void main() { ); expect( - rs.currentRoster!.indexWhere((item) => item.jid == 'testuser2@server2.example') == -1, + rs.getRosterItems().indexWhere((item) => item.jid == 'testuser2@server2.example') == -1, true, ); expect( - rs.currentRoster!.indexWhere((item) => item.jid == 'testuser@server.example') != 1, + rs.getRosterItems().indexWhere((item) => item.jid == 'testuser@server.example') != 1, true, ); expect(rs.loadCount, 1); - expect(rs.currentRoster!.length, 1); + expect(rs.getRosterItems().length, 1); }); test('Test a roster fetch', () async { @@ -85,9 +85,9 @@ void main() { ); expect(rs.loadCount, 1); - expect(rs.currentRoster!.length, 3); - expect(rs.currentRoster!.indexWhere((item) => item.jid == 'testuser@server.example') != -1, true); - expect(rs.currentRoster!.indexWhere((item) => item.jid == 'testuser2@server2.example') != -1, true); - expect(rs.currentRoster!.indexWhere((item) => item.jid == 'testuser3@server3.example') != -1, true); + expect(rs.getRosterItems().length, 3); + expect(rs.getRosterItems().indexWhere((item) => item.jid == 'testuser@server.example') != -1, true); + expect(rs.getRosterItems().indexWhere((item) => item.jid == 'testuser2@server2.example') != -1, true); + expect(rs.getRosterItems().indexWhere((item) => item.jid == 'testuser3@server3.example') != -1, true); }); }