elixir-explorer / adbc

Apache Arrow ADBC bindings for Elixir
https://arrow.apache.org/adbc/
Apache License 2.0
50 stars 16 forks source link

[WIP] release statement #52

Closed cocoa-xu closed 10 months ago

cocoa-xu commented 10 months ago

Try to release statement if possible. Related to issue #50.

josevalim commented 10 months ago

Instead of releasing it in the code, wouldn't it better to release it on GC via a NIF destructor? The reason I ask is because if the process crashes due a Process.exit(pid, :kill), terminate does not run and therefore it can leak.

cocoa-xu commented 10 months ago

It seems that the GC releases the statement earlier than we want/it should be in my test...

cocoa-xu commented 10 months ago

Now it should work

cocoa-xu commented 10 months ago

Instead of using templates, could we define a destruct resource per type? Because we should define a release statement for each Adbc*Release function we have, and not only statements. WDYT? Or are templates the only way to achieve this?

Yeah we can do that. But I found that adding a destructor for AdbcConnection will cause segfaults.

static void destruct_adbc_connection_resource(ErlNifEnv *env, void *args) {
  auto res = (NifRes<struct AdbcConnection> *)args;
  if (res && !res->freed) {
    struct AdbcError adbc_error{};
    AdbcConnectionRelease(&res->val, &adbc_error);
    res->freed = true;
  }
}
josevalim commented 10 months ago

Yeah we can do that. But I found that adding a destructor for AdbcConnection will cause segfaults.

Can you please push those changes? Then I can try to debug locally as well :)

cocoa-xu commented 10 months ago

Sure, I've pushed the code!

josevalim commented 10 months ago

Closing in favor of #53.