utopia-rise / godot-kotlin-jvm

Godot Kotlin JVM Module
MIT License
576 stars 39 forks source link

Remove GodotStatic, Singleton and Leak management from MemoryManager. #508

Open CedNaru opened 10 months ago

CedNaru commented 10 months ago

Previously Godot didn't have any way to remove existing instance_bindings from Object, leading to some additional code in the MemoryManager to handle singletons, objects stored in static instances and potential leaks as cyclical references between C++ and Kotlin were possible.

Godot 4.2 introduces a new method Object::free_binding_instance. With that method, we can easily unbind any C++ object and Kotlin wrapper remaining in the memory manager when the JVM is closing. The singletons needed to be a special case, as the native objects were freed by Godot only after the JVM module was finished, leaving their references in the memory managers. Refcounted references stored in static objects will no longer be stuck in a cyclical reference either, with the instance binding removed, and regular Object are supposed to be manually managed by design anyway, so it was kind of a bad practice to give them an autofreed feature.

With that out of the way, there is no longer a need to keep track of potential leaks on the JVM side. If leaks remain, the user is going to be notified by the regular Godot ObjectDB anyway and the potential causes would just be for regular Godot reasons not related to the JVM (like not freeing an Object).

Let's take the 4 potential cases:

  1. Object not leaking in both C++ and JVM: Everything is fine
  2. Object leaking in both C++ and JVM: The C++ side already has the responsibility of reporting those leaks, there is no need for the JVM to do the same work a second time.
  3. Object leaking in C++ but not in the JVM: Not our job.
  4. Object leaking in the JVM but not in C++: In such a case, it means that the native object was already freed but that the reference is still alive in the JVM. By design this case is impossible for Refcounted instances because if a JVM instance is still alive, then the C++ side is as well, and we are in case 2. The only possibility left then are regular Objects that have been manually freed earlier in the code but somehow still used in the Kotlin code. I think this last case is not our responsability either. Manual Object management has to be properly done in Godot regardless of the script language, this is up to the user to design a code that won't make use of any dead pointer. We also don't need to report on that, because if a dead instance was improperly used in the Kotlin code, then the user would have received an error from Godot way before the game closing.

Taking everything into account and with the new memory design allowed by 4.2. We have no reason to check for leaks on the Kotlin side (and currently making the game crash when it happens instead of just a warning like Godot does) so I would just remove that part.

Can anyone think of a possible case I didn't mention ?