Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Guard against potential usage of DrawablePool without being added to hierarchy #6136

Merged
merged 3 commits into from Jan 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 16 additions & 14 deletions osu.Framework.Tests/Visual/Drawables/TestSceneDrawablePool.cs
@@ -1,8 +1,6 @@
// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
// See the LICENCE file in the repository root for full licence text.

#nullable disable

using System;
using System.Collections.Generic;
using System.Linq;
Expand All @@ -22,8 +20,8 @@ namespace osu.Framework.Tests.Visual.Drawables
{
public partial class TestSceneDrawablePool : TestScene
{
private DrawablePool<TestDrawable> pool;
private SpriteText count;
private DrawablePool<TestDrawable> pool = null!;
private SpriteText? count;

private readonly HashSet<TestDrawable> consumed = new HashSet<TestDrawable>();

Expand Down Expand Up @@ -85,7 +83,7 @@ public void TestReturnWithoutAdding()
{
resetWithNewPool(() => new TestPool(TimePerAction, 1));

TestDrawable drawable = null;
TestDrawable drawable = null!;

AddStep("consume without adding", () => drawable = pool.Get());

Expand Down Expand Up @@ -113,7 +111,7 @@ public void TestPoolReturnWhenAboveCapacity()
{
resetWithNewPool(() => new TestPool(TimePerAction * 20, 1, 1));

TestDrawable first = null, second = null;
TestDrawable first = null!, second = null!;

AddStep("consume item", () => first = consumeDrawable());

Expand Down Expand Up @@ -142,8 +140,8 @@ public void TestPrepareAndFreeMethods()
{
resetWithNewPool(() => new TestPool(TimePerAction, 1));

TestDrawable drawable = null;
TestDrawable drawable2 = null;
TestDrawable drawable = null!;
TestDrawable drawable2 = null!;

AddStep("consume item", () => drawable = consumeDrawable());

Expand All @@ -165,8 +163,8 @@ public void TestPrepareOnlyOnceOnMultipleUsages()
{
resetWithNewPool(() => new TestPool(TimePerAction, 1));

TestDrawable drawable = null;
TestDrawable drawable2 = null;
TestDrawable drawable = null!;
TestDrawable drawable2 = null!;

AddStep("consume item", () => drawable = consumeDrawable(false));

Expand All @@ -189,7 +187,7 @@ public void TestPrepareOnlyOnceOnMultipleUsages()
[Test]
public void TestUsePoolableDrawableWithoutPool()
{
TestDrawable drawable = null;
TestDrawable drawable = null!;

AddStep("consume item", () => Add(drawable = new TestDrawable()));

Expand Down Expand Up @@ -266,7 +264,11 @@ public void TestPoolUsageExceedsMaximum(int maxPoolSize)
[Test]
public void TestGetFromNotLoadedPool()
{
Assert.DoesNotThrow(() => new TestPool(100, 1).Get());
// could theoretically be made to work - and did previously work -
// but it would work in a counterintuitive manner
// (as it will not preload the initial count of drawables in advance on load thread),
// so to avoid a footgun make this throw.
Assert.Throws<InvalidOperationException>(() => new TestPool(100, 1).Get());
}

/// <summary>
Expand All @@ -278,7 +280,7 @@ public void TestParentInvalidationFromChildDoesNotReturnPooledParent()
{
resetWithNewPool(() => new TestPool(TimePerAction, 1));

TestDrawable drawable = null;
TestDrawable drawable = null!;

AddStep("consume item", () => drawable = consumeDrawable(false));
AddStep("add child", () => drawable.AddChild(Empty()));
Expand All @@ -290,7 +292,7 @@ public void TestDrawablePreparedWhenClockRewound()
{
resetWithNewPool(() => new TestPool(TimePerAction, 1));

TestDrawable drawable = null;
TestDrawable drawable = null!;

AddStep("consume item and rewind clock", () =>
{
Expand Down
3 changes: 3 additions & 0 deletions osu.Framework/Graphics/Pooling/DrawablePool.cs
Expand Up @@ -111,6 +111,9 @@ public void Return(PoolableDrawable pooledDrawable)
/// <returns>The drawable.</returns>
public T Get(Action<T> setupAction = null)
{
if (LoadState <= LoadState.Loading)
throw new InvalidOperationException($"A {nameof(DrawablePool<T>)} must be in a loaded state before retrieving pooled drawables.");

if (!pool.TryPop(out var drawable))
{
drawable = create();
Expand Down