Bugs in UnityEngine.Pool.ObjectPool<T>

I recently tried writing an implementation matching to Unity 2021's

ObjectPool

for Unity 2020 LTS. Obviously, I didn't have access to the disassembled code before implementing my own version, so it is original in several areas.

However, at one point I did get my hands on the disassembled code for

UnityEngine.Pool.ObjectPool<T>

for checking if I had done most of my implementation correctly, balancing the logic as appropriate.

Not sharing the disassembled code since I am not sure if that's legal. One can use ILSpy or dotPeek to get a copy from the latest Unity 2021. I got a copy of the file from another person online.

However I noticed a few things:

Nobody's checking what kind of object is coming in inside

public void Release(T element)

. No null checks, no checks for whether the object was created inside the

ObjectPool

using

createFunc

or not.

Allowing the pool to contain

null

s is asking for trouble.

If you make an object outside the pool without using

createFunc

and release it to the pool, the

CountAll

will become inaccurate as it always counts up whenever a new object is made inside the pool. This also directly cascades into

CountActive

as per the code.

In my implementation, I use a separate

HashSet

(which is initialised conditionally at construction) to verify if any active objects have been created inside the

ObjectPool

or not. I also throw an error if the element is null. Here's my implementation:

(Edit: added

DEVELOPMENT_BUILD

directive check for silencing the script during release.)

Code (CSharp):

1. public void Release(T element)
2. {
3.     #region Checks
4.     // Throw error for null element
5.     if (element == null)
6.     {
7. #if DEVELOPMENT_BUILD
8.         throw new ArgumentNullException("Provided element was null.", nameof(element));
9. #else
10.         return;
11. #endif
12.     }
13.  
14.     // If activeItems is valid, throw an error if the element hasn't been created by this pool
15.     if (!activeItems?.Contains(element) ?? false)
16.     {
17. #if DEVELOPMENT_BUILD
18.         throw new InvalidOperationException($"Cannot release element which has not been created by this pool: {this}.");
19. #else
20.         return;
21. #endif
22.     }
23.  
24.     // If collectionCheck is enabled, throw an error if the element is already in the pool
25.     if (collectionCheck && CountInactive > 0 && pool.Contains(element))
26.     {
27. #if DEVELOPMENT_BUILD
28.         throw new InvalidOperationException($"Trying to release element already released to this pool: {this}.");
29. #else
30.         return;
31. #endif
32.     }
33.     #endregion // Checks
34.  
35.     // Store the instance if we're not at full capacity
36.     if (CountInactive < maxSize)
37.     {
38.         actionOnRelease?.Invoke(element);
39.         pool.Push(element);
40.         activeItems?.Remove(element);
41.     }
42.     else // Destroy it if we're full
43.     {
44.         actionOnDestroy?.Invoke(element);
45.     }
46. }
47.  

Please advise if this was the best way to report it.

 

The best way to report bugs is Help -> Report a Bug inside the Editor as that creates an actual ticket in the system.

https://unity3d.com/unity/qa/bug-reporting

 

Will perform this soon.

 

A few comments:

1. If you're using a pattern like

Code (CSharp):

1. using (SomePool.Get(out var someObject))
2. {
3.      //Do something with the object
4. }

you'd not be likely to Release a null unless you nulled someObject in that scope.

2. One can always do null-checks before Releasing an object to the pool or wrap the Restore with a method to check before Releasing. However, you're right that Releasing a null would be nasty. I had noticed this too but rather than a bug I had assumed that this was done for performance reasons since these pools seem to exist to support pooling of collection Types rather than pooling of prefabs or gameobjects (although I also use them that way myself). I check for null before using Release when the pattern isn't like #1 above and there's any possibility of a null. I'm not sure that an exception on Release(null) would be the best way to handle it (no real opinion though).

 

I'd not want

null

s to be given to users even if I were pooling collections (my personal implementation doesn't cover the collection specific Pool classes).

Besides I feel that not addressing the problem and relying on checking for

null

in my own code is not going to help someone who sees their code failing without knowing where their logic is wrong. Better address it systemically.

I can agree that throwing an exception is usually not preferred in production code (perhaps conditionally compiled only in Development/Debug). I'll change my function to do so too.

 

Unity's C# source code is publicly available, here.

Sadly, it's outdated, and I don't know what's happening with that, but it's probably the same code for the most part.

 

Indeed, but Unity 2021 code isn't available. Anyway, as long as it's used for reference and not copied outright it's okay, I believe.

 

Official response from Unity by Titas Vizgirda (bugs@unity3d.com):