firebase / firebase-unity-sdk

The Firebase SDK for Unity
http://firebase.google.com
Apache License 2.0
220 stars 35 forks source link

Feature Request: Build in safety for accessing the Firebase instance in Finalize - either as a warning or with some other guard against a deadlock lock. #397

Open puschie286 opened 4 years ago

puschie286 commented 4 years ago

Please fill in the following fields:

Unity editor version: 2020.1 Firebase Unity SDK version: 16.15.2 Source you installed the SDK (.unitypackage or Unity Package Manager): UPM Firebase plugins in use (Auth, Database, etc.): auth, database, messaging Additional SDKs you are using (Facebook, AdMob, etc.): does not matter Platform you are using the Unity editor on (Mac, Windows, or Linux): windows Platform you are targeting (iOS, Android, and/or desktop): desktop Scripting Runtime (Mono, and/or IL2CPP): both

Please describe the issue here:

the unity editor does not clean up the game classes when exiting play mode but right before a entering play mode and before a recompile. if you try to cleanup ( accessing firebase app or firebase database ) in the destructor of your class and the play mode has not started it will cause a deadlook of the whole editor.

callstack: image

Please answer the following, if applicable:

Have you been able to reproduce this issue with just the Firebase Unity quickstarts (this GitHub project)? no

What's the issue repro rate? (eg 100%, 1/5 etc) 100%

puschie286 commented 4 years ago

workaround is to put: if( FirebaseApp.GetInstance( FirebaseApp.DefaultName ) == null ) { return; } in all functions that can be called from the destructor ( if you need to access firebase app there )

patm1987 commented 4 years ago

Thanks for the report, and it seems pretty straightforward. Would you mind throwing together a quick script that I can drop in, say, the Realtime Database testapp so I can verify this quickly and throw on over to our internal bug tracker?

puschie286 commented 4 years ago

here is the whole test-app project because you cant upload .cs files testapp.zip

chkuang-g commented 4 years ago

@puschie286 Great profile picture btw. Love Ghost in Shell.

In general, it is not a good idea to access, especially create or destroy, a Firebase instance in a destructor. I would recommend you to use MonoBehaviour.OnApplicationQuit() or MonoBehavior.OnDestory() instead. You should be able to do something like

  void OnDestroy() {
    FirebaseDatabase.DefaultInstance.GetReference( ".info/connected" ).ValueChanged -= OnValueChanged;
  }

When you stop the play mode, Unity will try to tear down all Unity managed object, like GameObject and MonoBehavior, but not AppDomain. This sometimes results in some weird behavior if you want to do something in the destructor.

Let us know if this helps your situation.

google-oss-bot commented 4 years ago

Hey @puschie286. We need more information to resolve this issue but there hasn't been an update in 5 weekdays. I'm marking the issue as stale and if there are no new updates in the next 5 days I will close it automatically.

If you have more information that will help us get to the bottom of this, just add a comment!

puschie286 commented 4 years ago

Yes this resolve the problem I was hopping if you could throw a warning/exception when accessing firebase outside the unity lifecycle ? Needed a lot a debugging to find out that it’s related to firebase because it’s try to reinitialize the instance after it was destroyed - which looked right at first look but the usage of unity references caused a deadlock So a exception would make this much more obvious Maybe with some preprocessor directive ?

patm1987 commented 4 years ago

Since you've been able to work around this issue, I'm going to change this to a feature request (and update the subject line).

I will echo @chkuang-g that doing work in Finalize is generally not a great idea in C# or Unity. I'd definitely recommend OnDestroy() on a MonoBehaviour in the Unity world in general for maximum safety (but I acknowledge that there are valid use cases for what you're doing).