`ComponentPool` returns components to the pool while they are still in use, and loses components removed before mounting

What happened?

ComponentPool decides a component can be reused when its removed future completes (mounted.then → removed.then → _release). But removed doesn't mean the component has left the tree, so the pool gets four things wrong:

1. Moving a pooled component returns it to the pool while it's still mounted. A move goes through _handleLifecycleEventMove, which calls _remove(oldParent), completing removed, and then mounts the component under the new parent straight away. _release runs a microtask later and puts the still-mounted component in the pool, so the next acquire() hands out a component that's already in use.
2. Removing an ancestor returns its pooled descendants to the pool while they're still attached. _remove completes removed for every component in the subtree, but only the top one gets its _parent cleared. The descendants go back to the pool while still being children of the removed ancestor. Acquiring one of them and adding it again goes through the move path, which calls its onRemove a second time.
3. A component removed before it's mounted is never returned. Removing a component before it's mounted (for example in the same tick it was added) cancels its pending add, so mounted never completes and the release listener never fires. A component that's acquired and never added is lost too, since ComponentPool has no release().
4. A released component can't be reused in the same tick. The release runs in a microtask after the removal. A game that removes and spawns in the same frame, the typical bullet pattern, keeps calling the factory, and availableCount is too low until the microtasks run.

What do you expect?

A component should go back to the pool exactly when it is detached from the component tree, and synchronously at that moment:

• A move to a new parent doesn't return it.
• Removing an ancestor doesn't return it either. The component stays attached to that ancestor, and goes back to the pool only when it is itself removed.
• A component removed before mounting is returned.
• After removeFromParent() and the next lifecycle pass, the component can be acquired again in the same tick.

How can we reproduce this?

Each test below fails at the line marked // Fails:, with the value shown in the comment.

import 'package:flame/components.dart';
import 'package:flame_test/flame_test.dart';
import 'package:flutter_test/flutter_test.dart';

class _Pooled() extends PositionComponent {
  int removeCount = 0;

  @override
  void onRemove() {
    removeCount++;
    super.onRemove();
  }
}

ComponentPool<_Pooled> _createPool() =>
    ComponentPool<_Pooled>(factory: _Pooled.new);

void main() {
  testWithFlameGame('moving a component releases it', (game) async {
    final pool = _createPool();
    final a = Component();
    final b = Component();
    await game.ensureAddAll([a, b]);

    final component = pool.acquire();
    await a.ensureAdd(component);

    component.parent = b;
    await game.ready();

    expect(component.isMounted, isTrue);
    expect(component.parent, b);
    expect(pool.availableCount, 0); // Fails: 1
  });

  testWithFlameGame('acquire returns a mounted component', (game) async {
    final pool = _createPool();
    final a = Component();
    final b = Component();
    await game.ensureAddAll([a, b]);

    final component = pool.acquire();
    await a.ensureAdd(component);

    component.parent = b;
    await game.ready();

    expect(pool.acquire().isMounted, isFalse); // Fails: true
  });

  testWithFlameGame('removing an ancestor releases a child', (game) async {
    final pool = _createPool();
    final container = Component();
    await game.ensureAdd(container);

    final component = pool.acquire();
    await container.ensureAdd(component);

    container.removeFromParent();
    await game.ready();

    expect(component.parent, container);
    expect(pool.availableCount, 0); // Fails: 1
  });

  testWithFlameGame('re-adding that child calls onRemove again', (
    game,
  ) async {
    final pool = _createPool();
    final container = Component();
    await game.ensureAdd(container);

    final component = pool.acquire();
    await container.ensureAdd(component);

    container.removeFromParent();
    await game.ready();
    expect(component.removeCount, 1);

    final reacquired = pool.acquire();
    expect(identical(reacquired, component), isTrue);
    game.add(reacquired);
    await game.ready();

    expect(reacquired.parent, game);
    expect(reacquired.removeCount, 1); // Fails: 2
  });

  testWithFlameGame('removed before mounting is lost', (game) async {
    final pool = _createPool();

    final component = pool.acquire();
    game.add(component);
    component.removeFromParent();
    await game.ready();

    expect(component.isMounted, isFalse);
    expect(pool.availableCount, 1); // Fails: 0
  });

  testWithFlameGame('not available within the same tick', (game) async {
    final pool = _createPool();

    final component = pool.acquire();
    await game.ensureAdd(component);

    component.removeFromParent();
    game.update(0);

    expect(component.isMounted, isFalse);
    expect(pool.availableCount, 1); // Fails: 0
  });
}

What steps should take to fix this?

This can't be fixed from inside the pool. Listening for removed misses detaches that complete no future at all (such as a cancelled pending add), and futures can never complete in the same tick. A check on whether the component is still attached, followed by listening again, would fix the move case only.

The fix will add a private, synchronous detach notification to Component. It fires when a component is removed, or removed before it could be mounted, and not on a move or for the descendants of a removed component. ComponentPool takes components back through it.

How a component is linked to its pool is still to be decided:

• A PooledComponent mixin. ComponentPool<T extends PooledComponent> stores the pool reference on the component and provides onAcquire/onRelease hooks to override. This is a breaking change.
• An Expando in the component library, with nothing else changing for users. ComponentPool<T extends Component> stays as it is, components that aren't pooled use no extra memory, and the reset hooks become optional onAcquire/onRelease callbacks on the pool.

Along with the fix:

• ComponentPool.release(T) for components that were acquired but never added to the tree.
• onAcquire, so that reset logic stops depending on onMount.
• Asserts against negative sizes, a double release, a release from a different pool, and a component that's still in the pool getting added to the tree.
• Drop the O(n) _available.contains check.
• Regression tests for all four cases.

Do have an example of where the bug occurs?

The tests above. The ComponentPool example in examples/ doesn't hit these cases, because its balls are only ever added to and removed from the world.

Relevant log output

00:00 +0 -1: moving a component releases it [E]
  Expected: <0>
    Actual: <1>
00:00 +0 -2: acquire returns a mounted component [E]
  Expected: false
    Actual: <true>
00:00 +0 -3: removing an ancestor releases a child [E]
  Expected: <0>
    Actual: <1>
00:00 +0 -4: re-adding that child calls onRemove again [E]
  Expected: <1>
    Actual: <2>
00:00 +0 -5: removed before mounting is lost [E]
  Expected: <1>
    Actual: <0>
00:00 +0 -6: not available within the same tick [E]
  Expected: <1>
    Actual: <0>
00:00 +0 -6: Some tests failed.

Execute in a terminal and put output into the code block below

N/A, not platform specific. Reproduced on main at 31398234e.

Affected platforms

All

Are you interested in working on a PR for this?

• I want to work on this