fix: Fix priority rebalancing causing concurrent mutation of component ordered_set (#3530)

Fix priority rebalancing causing concurrent mutation of component
ordered_set.

This fixes #3363

Basically priority changes and rebalancing was not using the component
lifecycle event queue, which caused concurrent mutations and iterations
of the component set, and due to race conditions, loss of components as
they are removed and re-added for rebalancing.

The issue is not with ordered_set itself, but rather how it is being
used. It was never designed to support concurrent mutations.

A few notes for the future:

* I think we should consider changing the whole event (input) system so
that every event is queued and only processed predictably within the
game loop, avoiding a whole class of concurrent modification issues
* we should consider more efficient ways of rebalancing than literally
removing every single component + re-adding (which is also error prone
if there is concurrent access).

---------

Co-authored-by: Lukas Klingsbo <[email protected]>

Author: luanpotter

.github/.cspell/words_dictionary.txt

@@ -23,6 +23,7 @@ prerendered
 pressable
 ptero # short for pterodactyl
 rebalance
+rebalancing
 refreshable
 renderable
 rescan

packages/flame/lib/src/components/core/component.dart

@@ -819,10 +819,12 @@ class Component {
   int _priority;
   set priority(int newPriority) {
     if (_priority != newPriority) {
-      _priority = newPriority;
+      final parent = _parent;
       final game = findGame();
-      if (game != null && _parent != null) {
-        game.enqueueRebalance(_parent!);
+      if (game != null && parent != null) {
+        game.enqueuePriorityChange(parent, this, newPriority);
+      } else {
+        _priority = newPriority;
       }
     }
   }
@@ -877,6 +879,16 @@ class Component {
     return LifecycleEventStatus.done;
   }
 
+  /// NOTE: this method will intentionally not reorder the parent,
+  /// leaving the ordered set in an inconsistent state.
+  /// It is left to the caller to optimally reorder the parent.
+  @internal
+  void handleLifecycleEventRebalanceUncleanly(int newPriority) {
+    if (_priority != newPriority) {
+      _priority = newPriority;
+    }
+  }
+
   @mustCallSuper
   @internal
   void handleResize(Vector2 size) {

packages/flame/lib/src/components/core/component_tree_root.dart

@@ -15,12 +15,10 @@ class ComponentTreeRoot extends Component {
     super.children,
     super.key,
   })  : _queue = RecycledQueue(_LifecycleEvent.new),
-        _blocked = <int>{},
-        _componentsToRebalance = <Component>{};
+        _blocked = <int>{};
 
   final RecycledQueue<_LifecycleEvent> _queue;
   final Set<int> _blocked;
-  final Set<Component> _componentsToRebalance;
   late final Map<ComponentKey, Component> _index = {};
   Completer<void>? _lifecycleEventsCompleter;
 
@@ -73,8 +71,16 @@ class ComponentTreeRoot extends Component {
   }
 
   @internal
-  void enqueueRebalance(Component parent) {
-    _componentsToRebalance.add(parent);
+  void enqueuePriorityChange(
+    Component parent,
+    Component child,
+    int newPriority,
+  ) {
+    _queue.addLast()
+      ..kind = _LifecycleEventKind.rebalance
+      ..child = child
+      ..parent = parent
+      ..newPriority = newPriority;
   }
 
   bool get hasLifecycleEvents => _queue.isNotEmpty;
@@ -107,6 +113,17 @@ class ComponentTreeRoot extends Component {
   }
 
   void processLifecycleEvents() {
+    // reorder events to process later grouped by parent
+    final reorderEventsByParent = <Component, List<(Component, int)>>{};
+    LifecycleEventStatus handleReorderEvent(
+      Component parent,
+      Component child,
+      int newPriority,
+    ) {
+      (reorderEventsByParent[parent] ??= []).add((child, newPriority));
+      return LifecycleEventStatus.done;
+    }
+
     assert(_blocked.isEmpty);
     var repeatLoop = true;
     while (repeatLoop) {
@@ -124,6 +141,8 @@ class ComponentTreeRoot extends Component {
           _LifecycleEventKind.remove =>
             child.handleLifecycleEventRemove(parent),
           _LifecycleEventKind.move => child.handleLifecycleEventMove(parent),
+          _LifecycleEventKind.rebalance =>
+            handleReorderEvent(parent, child, event.newPriority!),
           _LifecycleEventKind.unknown => LifecycleEventStatus.done,
         };
 
@@ -140,19 +159,20 @@ class ComponentTreeRoot extends Component {
       _blocked.clear();
     }
 
+    for (final MapEntry(key: parent, value: events)
+        in reorderEventsByParent.entries) {
+      for (final (child, newPriority) in events) {
+        child.handleLifecycleEventRebalanceUncleanly(newPriority);
+      }
+      parent.children.reorder();
+    }
+
     if (!hasLifecycleEvents && _lifecycleEventsCompleter != null) {
       _lifecycleEventsCompleter!.complete();
       _lifecycleEventsCompleter = null;
     }
   }
 
-  void processRebalanceEvents() {
-    for (final component in _componentsToRebalance) {
-      component.children.reorder();
-    }
-    _componentsToRebalance.clear();
-  }
-
   @mustCallSuper
   @override
   @internal
@@ -207,18 +227,21 @@ enum _LifecycleEventKind {
   add,
   remove,
   move,
+  rebalance,
 }
 
 class _LifecycleEvent implements Disposable {
   _LifecycleEventKind kind = _LifecycleEventKind.unknown;
   Component? child;
   Component? parent;
+  int? newPriority;
 
   @override
   void dispose() {
     kind = _LifecycleEventKind.unknown;
     child = null;
     parent = null;
+    newPriority = null;
   }
 
   @override

packages/flame/lib/src/game/flame_game.dart

@@ -162,7 +162,6 @@ class FlameGame<W extends World> extends ComponentTreeRoot
     for (final component in children) {
       component.updateTree(dt);
     }
-    processRebalanceEvents();
   }
 
   /// This passes the new size along to every component in the tree via their

packages/flame/test/components/component_test.dart

@@ -1,3 +1,5 @@
+import 'dart:math';
+
 import 'package:flame/components.dart';
 import 'package:flame/game.dart';
 import 'package:flame_test/flame_test.dart';
@@ -1015,6 +1017,72 @@ void main() {
       });
     });
 
+    group('Rebalancing components', () {
+      testWithFlameGame(
+        'rebalance is queued',
+        (game) async {
+          final c = Component();
+          await game.world.add(c);
+
+          c.priority = 10;
+          expect(c.priority, 0);
+
+          await game.ready();
+          expect(c.priority, 10);
+        },
+      );
+
+      testWithFlameGame(
+        'async rebalance, add and remove have no race condition',
+        (game) async {
+          final r = Random(69420);
+          for (var i = 0; i < 10; i++) {
+            game.world.add(Component());
+          }
+
+          Future<void> add() async {
+            final white = Component();
+            game.world.add(white);
+            await Future.delayed(
+              const Duration(milliseconds: 300),
+              white.removeFromParent,
+            );
+          }
+
+          Future<void> rebalance() async {
+            game.world.children.forEach((it) {
+              it.priority = r.nextInt(1_000);
+            });
+          }
+
+          var completed = 0;
+          var total = 0;
+          void waitFor(int milliseconds, Future<void> Function() fn) {
+            total++;
+            Future.delayed(Duration(milliseconds: milliseconds), fn)
+                .whenComplete(() => completed++);
+          }
+
+          Future<void> start() async {
+            for (var i = 0; i < 100; i++) {
+              waitFor(17 * i, rebalance);
+              waitFor(31 * i, add);
+            }
+          }
+
+          waitFor(0, start);
+
+          while (completed < total || game.hasLifecycleEvents) {
+            game.update(0);
+            await Future.delayed(const Duration(milliseconds: 1));
+          }
+
+          await game.ready();
+          expect(game.world.children.length, 10);
+        },
+      );
+    });
+
     group('descendants()', () {
       testWithFlameGame(
         'descendants in a deep component tree',

packages/flame/test/components/priority_test.dart

@@ -149,6 +149,7 @@ void main() {
 
         a.priority = 10;
         game.update(0);
+        await game.ready();
 
         componentsSorted(game.children);
         componentsSorted(a.children);
@@ -163,6 +164,7 @@ void main() {
         c1.priority = 10;
         a2.priority = 0;
         game.update(0);
+        await game.ready();
 
         a.assertCalled(1);
         b.assertCalled(0);
@@ -177,6 +179,7 @@ void main() {
         b1.priority = 2;
         a1.priority = 1; // no-op!
         game.update(0);
+        await game.ready();
 
         a.assertCalled(0);
         b.assertCalled(1);
@@ -217,7 +220,10 @@ void main() {
 
       expect(parent.priority, 0);
       expect(child.priority, 0);
+
       game.update(0.1);
+      await game.ready();
+
       expect(parent.priority, 10);
       expect(child.priority, 0);
       expect(renderEvents, isEmpty);

packages/flame/test/components/router_component_test.dart

@@ -251,15 +251,18 @@ void main() {
       expect(router.stack.length, 1);
 
       router.pushNamed('B');
+      await game.ready();
       router.pushNamed('C');
+      await game.ready();
       expect(router.stack.length, 3);
 
       router.pushNamed('B');
+      await game.ready();
       expect(router.stack.length, 3);
       router.pushNamed('A');
+      await game.ready();
       expect(router.stack.length, 3);
 
-      await game.ready();
       expect(router.children.length, 3);
       expect((router.children.elementAt(0) as Route).name, 'C');
       expect((router.children.elementAt(1) as Route).name, 'B');