Mixedrealitytoolkit-unity: Improper use of Singleton.Instance in OnDisable / OnDestroy

[arctop-sk]

Overview

Singleton class contains an IsInitialized property to check if there is a valid instance, without invoking an instantiation, like Singleton.Instance does. Some class check in OnDisable/OnDestroy if Instance != null, which can cause leaking of objects.

Should change all code that wishes to check if an instance is actually created to use IsInitialized instead.

[StephenHodgson]

@arctop-sk, could you go into more detail about how exactly this leaks objects?

Do you have specific repro steps and a way to test this I can do?

[arctop-sk]

My apologies for the lacking description, I've had this fixed on my end for a while and forgot to raise the issue. In General, there are OnDestroy functions, that will follow the pattern of asking:

if (Singleton.instance != null) { remove listener; }

This could also be an issue when doing those in OnDisabled(), since the order of calls can sometimes bite you.

One example I found right now is in ImportExportAnchorManager
I first came across it in one of the Input scripts, that would cause the Input Manager to be re-instantiated.
If a singleton has been destroyed, and then an object that wants to deregister calls Instance, it will get re-instantiated, and you'll get an error message saying that objects have been leaked to the scene.

In general, checking if Singleton.Instance is null should never happen, since it will always be created, and hence defeats the purpose.

[StephenHodgson]

Agreed, I'll be sure to take a pass at everywhere we're using it and update usages accordingly.

Good thing in vNEXT we're moving away from singleton.

[SimonDarksideJ]

Which is thankfully in only ONE class :D at least for the core project.
And Agreed, Singleton.Instance should NEVER return null

[StephenHodgson]

I wonder if we can create a custom error that get's thrown by VS if someone checks null against that class' Instance property.

[arctop-sk]

I think a good way would be to mark Instance as Obselete and add a description to point users to either IsInitilaized or a new GetOrCreateInstance() function, so that everything is more verbose. I would probably even replace IsInitialized with HasInstance, for an even more descriptive naming.
In general using Singltons in unity is something to avoid as much as possible, especially since the DontDestroyOnLoad must be applied to a root GameObject, and is really bug prone when trying to work with multiple scenes.

[StephenHodgson]

Hmm. Maybe. But that's too much of a breaking change. We can def implement something similar in vNEXT tho.

[davidkline-ms]

To fix this for the master branch, we could potentially add a GameObject to the scene and attach the appropriate script if searchForInstance fails. That should solve the Instance returning null issue.

[StephenHodgson]

Not sure how that fixes?

Instance never returns null, which is the problem.

[davidkline-ms]

Was responding to @SimonDarksideJ's "Singleton.Instance should NEVER return null".

I think I must've misread something earlier on.

[wiwei]

Closing issues older than 180 days. If this is still an issue, please reactivate with recent information.