godotengine / godot-cpp

C++ bindings for the Godot script API
MIT License
1.75k stars 580 forks source link

Pool*Arrays leaks #223

Open bruvzg opened 5 years ago

bruvzg commented 5 years ago

Pool arrays created in gdscript and passed to gdnative module are leaking. Or am I missing something and pool arrays should be freed manually?

gdnative module code:

void GDNObj::set_data(const PoolByteArray p_data) {

    if (p_data.size() == 0)
        return;

    if (data)
        std::free(data);

    data = std::malloc(p_data.size());
    std::memcpy(data, p_data.read().ptr(), p_data.size());
}

gdscript code:

func _ready():
    var gdn_obj = GDNObj.new()
    var bytes = PoolByteArray([0x41, 0x00, ... , 0x74, 0x00])
    gdb_obj.set_data(bytes)

Results in following error message at exit:

ERROR: cleanup: There are still MemoryPool allocs in use at exit!
   At: core/dvector.cpp:70.
Ovnuniarchos commented 4 years ago

Confirmed in commit 7cbb846. Any PoolArray, even if it's not accessed, will leak as soon as a change is made on any of the PAs (GDS side or GDN side).

Ovnuniarchos commented 4 years ago

Still failing in commit 9eceb16 , but only changes made on the GDScript side trigger the leak.

follower commented 4 years ago

@Ovnuniarchos Do you have a full example code that includes "changes made on the GDScript side"?

I was encountering the AFAICT original issue which appears to be gone in https://github.com/godotengine/godot-cpp/commit/9eceb16f0553884094d0f659461649be5d333866 (although I didn't try @bruvzg's specific example) so I wondered what was different with what you were observing and if I understood what you meant by "changes made on the GDScript side".

Related to what I'm trying to summarize here: https://github.com/godotengine/godot-cpp/issues/417

2shady4u commented 3 years ago

@follower I'm still having this issue on the latest master for my SQLite plugin: https://github.com/2shady4u/godot-sqlite/tree/feature-add-blobs

2

Capture

So I don't think it's fixed yet? 🤔

Zylann commented 3 years ago

I tried reproducing it on Windows but was unable to do so. I used a library compiled in debug mode. I tested in Godot 3.2.3 stable and some version of Godot 3.2.4 dated from december 2020. Using -v for verbose output, nothing showed up.

All my code is doing is this:

func test_pool_byte_array(): # Called from _ready
    print("BEGIN test_pool_byte_array")
    var test = CustomReference.new()
    var data = PoolByteArray([1, 2, 3])
    test.test_pool_byte_array(data)
    # data[0] = 42 # Also tried this, no problem
    print("END test_pool_byte_array")

C++:

void CustomReference::test_pool_byte_array(godot::PoolByteArray data) {
    PoolByteArray::Read r = data.read();
}

To properly figure this out, we need an actual minimal reproduction project (MRP), because it's still not clear under which circumstances this problem occurs (if it still occurs).

FYI there was a PR back in december 2019 which fixed a leak in PoolArrays: https://github.com/godotengine/godot-cpp/pull/355

Zylann commented 3 years ago

Update: we found various cases of leaks when some methods of Dictionary and Array are used, like values() or duplicate(). The leak then becomes apparent when they contain either poolarrays or resources (and curiously doesn't otherwise). However, we could not find an issue with PoolArrays yet.

2shady4u commented 3 years ago

Seems like my example is faulty as it pertains to having a PoolArray or Resource in an Array and then duplicating it (or Dictionaryand values()), as mentioned by @Zylann in the post above