ConcurrentModificationError using changeParent

[tboyce021]

Current bug behaviour

Calling changeParent throws a ConcurrentModificationError.

Expected behaviour

Calling changeParent shouldn't throw an exception.

Steps to reproduce

Run the following game and wait 5 seconds:

import 'package:flame/components.dart';
import 'package:flame/game.dart';
import 'package:flutter/widgets.dart';

void main() {
  runApp(
    GameWidget(
      game: ChangeParentGame(),
    ),
  );
}

class ChangeParentGame extends FlameGame {
  @override
  Future<void>? onLoad() async {
    final childComponent = Component();
    final parentComponent = Component();

    add(childComponent);
    add(parentComponent);

    Future.delayed(
      const Duration(seconds: 5),
      () => childComponent.changeParent(parentComponent),
    );
  }
}

Flutter doctor output

[√] Flutter (Channel stable, 3.0.5, on Microsoft Windows [Version 10.0.22000.856], locale en-US)
    • Flutter version 3.0.5 at D:\Users\username\AppData\Local\Programs\flutter
    • Upstream repository https://github.com/flutter/flutter.git
    • Framework revision f1875d570e (5 weeks ago), 2022-07-13 11:24:16 -0700
    • Engine revision e85ea0e79c
    • Dart version 2.17.6
    • DevTools version 2.12.2

[√] Android toolchain - develop for Android devices (Android SDK version 32.0.0-rc1)
    • Android SDK at D:\Users\username\AppData\Local\Android\sdk
    • Platform android-32, build-tools 32.0.0-rc1
    • Java binary at: D:\Program Files\Android\Android Studio\jre\bin\java
    • Java version OpenJDK Runtime Environment (build 11.0.12+7-b1504.28-7817840)
    • All Android licenses accepted.

[√] Chrome - develop for the web
    • Chrome at C:\Program Files\Google\Chrome\Application\chrome.exe

[√] Visual Studio - develop for Windows (Visual Studio Community 2022 17.3.0)
    • Visual Studio at C:\Program Files\Microsoft Visual Studio\2022\Community
    • Visual Studio Community 2022 version 17.3.32804.467
    • Windows 10 SDK version 10.0.19041.0

[√] Android Studio (version 2021.2)
    • Android Studio at C:\Program Files\Android\Android Studio
    • Flutter plugin can be installed from:
       https://plugins.jetbrains.com/plugin/9212-flutter
    • Dart plugin can be installed from:
       https://plugins.jetbrains.com/plugin/6351-dart
    • Java version OpenJDK Runtime Environment (build 11.0.12+7-b1504.28-7817840)

[√] Connected device (3 available)
    • Windows (desktop) • windows • windows-x64    • Microsoft Windows [Version 10.0.22000.856]
    • Chrome (web)      • chrome  • web-javascript • Google Chrome 104.0.5112.101
    • Edge (web)        • edge    • web-javascript • Microsoft Edge 104.0.1293.54

[√] HTTP Host Availability
    • All required HTTP hosts are available

• No issues found!

More environment information

Log information

Enter log information in this code block

More information

[spydon]

You should not modify the component tree from within a timer (it can run at any time), set a boolean or something from the timer and then do the actual modification in update.

[tboyce021]

I actually tried that but it's the same result:

class ChangeParentGame extends FlameGame {
  bool shouldChange = false;

  final childComponent = Component();
  final parentComponent = Component();

  @override
  Future<void>? onLoad() async {
    add(childComponent);
    add(parentComponent);

    Future.delayed(const Duration(seconds: 5), () => shouldChange = true);
  }

  @override
  void update(double dt) {
    super.update(dt);
    if (shouldChange) {
      shouldChange = false;
      childComponent.changeParent(parentComponent);
    }
  }
}

[tboyce021]

FWIW, the same code using the flag doesn't crash with flame 1.0.0 but does with 1.1.0+.

[tboyce021]

It seems to only be an issue when the child component was previously a sibling of the new parent or a sibling of an ancestor of the new parent.

This works (based on the tests)

wrapperComponent.add(childComponent);
add(wrapperComponent);
add(parentComponent);
...
childComponent.changeParent(parentComponent);

This also works:

parentWrapper.add(parentComponent);
childWrapper.add(childComponent);
add(parentWrapper);
add(childWrapper);

These don't:

add(childComponent);
add(parentComponent);

add(childComponent);
wrapperComponent.add(parentComponent);
add(wrapperComponent);

wrapperComponent.add(parentComponent);
wrapperComponent.add(childComponent);
add(wrapperComponent);

[spydon]

Very strange, since that feels like quite a common usecase, but it's probably a bug then. Could your write up some full tests that fail, that we can have a look at?

[st-pasha]

    testWithFlameGame('#1851', (game) async {
      final componentA = Component();
      final componentB = Component();
      game.addAll([componentA, componentB]);
      await game.ready();

      componentA.parent = componentB;
      game.update(0);
    });

dart:collection                               IterableMixin.forEach
package:flame/src/game/flame_game.dart 82:14  FlameGame.updateTree
package:flame/src/game/flame_game.dart 70:7   FlameGame.update
test/components/component_test.dart 680:12    main.<fn>.<fn>

Concurrent modification during iteration: _LinkedHashSet len:1.

[st-pasha]

I think I know what happens here. As we traverse the component tree during the update phase, we process the lifecycle events for each component, and then execute the update() and recurse into the children. However, if the child's lifecycle attempts to move that component to a different parent -- the one that is currently on the call stack, then a concurrent modification occurs.

Not yet sure how to fix.

[wolfenrain]

I think I know what happens here. As we traverse the component tree during the update phase, we process the lifecycle events for each component, and then execute the update() and recurse into the children. However, if the child's lifecycle attempts to move that component to a different parent -- the one that is currently on the call stack, then a concurrent modification occurs.

Not yet sure how to fix.

Should we not do the all the lifecycle events before the update, then do the update that can create new lifecycle events but those should only be consumed in the next tick?

[st-pasha]

I was thinking along the same lines: instead of having LifecycleManager on each component, make a single global lifecycle queue which would store pending lifecycle events for all components and then resolve them in bulk at the beginning of each game tick. This might be even more efficient, since the number of such events on every tick is not very big.