feat: Document the RosterStateManager better

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<XmppRosterItem> 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<XmppRosterItem>? currentRoster;
+  /// The cached version of the roster. If null, then it has not been loaded yet.
-  String? currentVersion;
+  List<XmppRosterItem>? _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<RosterCacheLoadResult> 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<void> commitRoster(String? version, List<String> removed, List<XmppRosterItem> modified, List<XmppRosterItem> added);
 
+  /// Load and cache or return the cached roster version.
   Future<String?> 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<void> _loadRosterCache() async {
-    if (currentRoster == null) {
+    if (_currentRoster == null) {
       final result = await loadRosterCache();
 
-      currentRoster = result.roster;
+      _currentRoster = result.roster;
-      currentVersion = result.version;
+      _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<void> handleRosterPush(RosterPushEvent event) async {
-    await _loadRosterCache();
+    await _lock.synchronized(() async {
+      await _loadRosterCache();
 
-    currentVersion = event.ver;
+      _currentVersion = event.ver;
-    final result = _handleRosterItem(event.item);
+      final result = _handleRosterItem(event.item);
 
-    if (result.removed != null) {
+      if (result.removed != null) {
-      return commitRoster(
+        return commitRoster(
-        currentVersion,
+          _currentVersion,
-        [result.removed!],
+          [result.removed!],
-        [],
+          [],
-        [],
+          [],
-      );
+        );
-    } else if (result.modified != null) {
+      } else if (result.modified != null) {
-      return commitRoster(
+        return commitRoster(
-        currentVersion,
+          _currentVersion,
-        [],
+          [],
-        [result.modified!],
+          [result.modified!],
-        [],
+          [],
-      );
+        );
-    } else if (result.added != null) {
+      } else if (result.added != null) {
-      return commitRoster(
+        return commitRoster(
-        currentVersion,
+          _currentVersion,
-        [],
+          [],
-        [],
+          [],
-        [result.added!],
+          [result.added!],
-      );
+        );
-    }
+      }
+    });
   }
 
+  /// Handles the result from a roster fetch.
   Future<void> handleRosterFetch(RosterRequestResult result) async {
-    final removed = List<String>.empty(growable: true);
+    await _lock.synchronized(() async {
-    final modified = List<XmppRosterItem>.empty(growable: true);
+      final removed = List<String>.empty(growable: true);
-    final added = List<XmppRosterItem>.empty(growable: true);
+      final modified = List<XmppRosterItem>.empty(growable: true);
+      final added = List<XmppRosterItem>.empty(growable: true);
 
-    await _loadRosterCache();
+      await _loadRosterCache();
 
-    currentVersion = result.ver;
+      _currentVersion = result.ver;
-    for (final item in result.items) {
+      for (final item in result.items) {
-      final result = _handleRosterItem(item);
+        final result = _handleRosterItem(item);
 
-      if (result.removed != null) removed.add(result.removed!);
+        if (result.removed != null) removed.add(result.removed!);
-      if (result.modified != null) modified.add(result.modified!);
+        if (result.modified != null) modified.add(result.modified!);
-      if (result.added != null) added.add(result.added!);
+        if (result.added != null) added.add(result.added!);
-    }
+      }
 
-    await commitRoster(
+      await commitRoster(
-      currentVersion,
+        _currentVersion,
-      removed,
+        removed,
-      modified,
+        modified,
-      added,
+        added,
-    );
+      );
+    });
   }
+
+  @visibleForTesting
+  List<XmppRosterItem> getRosterItems() => _currentRoster!;
 }
 
 @visibleForTesting

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.getRosterItems().length, 3);
-    expect(rs.currentRoster!.indexWhere((item) => item.jid == 'testuser@server.example') != -1, true);
+    expect(rs.getRosterItems().indexWhere((item) => item.jid == 'testuser@server.example') != -1, true);
-    expect(rs.currentRoster!.indexWhere((item) => item.jid == 'testuser2@server2.example') != -1, true);
+    expect(rs.getRosterItems().indexWhere((item) => item.jid == 'testuser2@server2.example') != -1, true);
-    expect(rs.currentRoster!.indexWhere((item) => item.jid == 'testuser3@server3.example') != -1, true);
+    expect(rs.getRosterItems().indexWhere((item) => item.jid == 'testuser3@server3.example') != -1, true);
   });
 }