cellml / libcellml

Repository for libCellML development.
https://libcellml.org
Apache License 2.0
16 stars 21 forks source link

JavaScript bindings: might want to add a `delete()` method to the classes that have a `create()` method #1238

Closed agarny closed 3 months ago

agarny commented 3 months ago

When you want to create a variable from JavaScript, you will do something like const v = new libcellml.Variable() and when you are done with v, you will typically call v.delete() since you can't rely on garbage collection working reliably (see https://github.com/emscripten-core/emscripten/issues/20569). This will mark v for later deletion.

This is also what I do in libOpenCOR, but to call .delete() never seems to call my object's destructor. So, in the end, I had to create a special delete() method for every class that I can create/delete using new/delete() from JavaScript. For instance, for my File class, I have:

// file.h

#ifdef __EMSCRIPTEN__
    void delete_();
#endif
// file.cpp

#ifdef __EMSCRIPTEN__
void File::delete_()
{
    delete this;
}
#endif
// JavaScript bindings

emscripten::class_<libOpenCOR::File, emscripten::base<libOpenCOR::Logger>>("File")
    .smart_ptr_constructor("File", &libOpenCOR::File::create)
    .function("delete", &libOpenCOR::File::delete_)
    ...
agarny commented 3 months ago

Hmm... I am going to leave this issue open, but it may not be a good way forward. In the test where I "need" that delete() method to delete the object straightaway, it all works fine probably but it looks like it might be because I do other things after calling my delete() method on some objects. Indeed, in some other tests, I finish the test with a call to my delete() method and it crashes with something like:

RuntimeError: table index is out of bounds

  at null.<anonymous> (wasm:/wasm/00fdfeea:1:127342)
  at null.<anonymous> (wasm:/wasm/00fdfeea:1:164484)
  at runDestructor (libopencor.js:3931:25)
  at releaseClassHandle (libopencor.js:3940:9)
  at libopencor.js:4090:9
      at FinalizationRegistry.cleanupSome (<anonymous>)

So, it looks like garbage collection is eventually doing its thing. So, I am starting to think that I have to rely on GC working fine, maybe just not as soon as I wish it did.

agarny commented 3 months ago

Having had time to think a bit more about it, I feel like we can close this issue. This is clearly the way things work in JavaScript, so... so be it.