Fix segfaults caused by closures passed to `Thread` methods

[dsander]

I sadly still don't really understand how the changes work, but did my best explaining my though process, which I hope makes sense 😄

---

Fix segfaults when calling Thread::call_without_gvl and call_with_gvl

I can't really explain why requiring FnMut over FnOnce fixes the problems described in #69. I only discovered that all examples for passing closure to C callbacks were using FnMut and not FnOnce. Based on the stacktraces I saw I am assuming that the compiler captures and returns the variables differently which somehow extends(?) their lifetime.

Moving the func into the wrap_return seems to be required to ensure the return value lives long enough.

Due to the additional requirements of FnMut the 'static lifetime of the closures are not required anymore. They have been introduced to fix this issue. The original example now also does not compile and requires str to be cloned. Done that it compiles and runs without a segfault.

---

Fix creating Thread::new when passing a closure capturing variables

Without requiring FnMut closures creating a ruby thread using a closure capturing variables segfaulted.

Fixes #69

[danielpclark]

I can't really explain why requiring FnMut over FnOnce fixes the problems described in #69. I only discovered that all examples for passing closure to C callbacks were using FnMut and not FnOnce.

This PR is similar in nature to https://github.com/danielpclark/rutie/pull/19 and he says there:

FnOnce consumes the variables. In other words, FnOnce may contain releasing resources.

Because he said that I think you have the right idea for this fix.

Hey @irxground , since this PR is similar in nature to one of yours #19, could you do me a favor and briefly look over this? I would really appreciate your wisdom on this since you have already shown yourself to have more understanding on this particular issue. Thanks in advance. :smiley:

The files in question are the example itself examples/rutie_ruby_gvl_example/src/lib.rs and then four different files which all have the same basic changes of exchanging FnOnce to FnMut:

• src/binding/thread.rs
• src/binding/vm.rs
• src/class/thread.rs
• src/util.rs

On key difference from your PR @irxground is the use of adding move on closures rather than perhaps unsafe blocks. Would the move now then imply Rust “may contain releasing resources” in FnMut?

[irxground]

The reason why the previous code does not work as expected seems to be a mistake of type.

binding::thread::call_without_gvl receives FnOnce as an argument. https://github.com/danielpclark/rutie/blob/60d22b206cfd7afff26e2df4f640479e5a391071/src/binding/thread.rs#L30-L32

However, thread_call_callbox is passed to rb_thread_call_without_gvl, and this callback function assumes FnMut. https://github.com/danielpclark/rutie/blob/60d22b206cfd7afff26e2df4f640479e5a391071/src/binding/thread.rs#L106-L107

This pull request modifies the type, so it will be working properly

[danielpclark]

@irxground Thanks! I really appreciate it. :+1:

[irxground]

I am sorry if I misunderstood your intention.

This issue and #19 are different. vm::protect is a function to wrap functions that cause Ruby exceptions. A memory leak will occur if resources are reserved in functions inside vm::protect.

There are three main types of resources of function objects. a) Resources created outside the function and borrowed by the function b) Resources created outside the function and consumed in the function c) Resources created inside functions

If a Ruby exception occurs in vm::protect, a) No memory leak occurs b) FnMut has no problem but FnOnce leaks memory. c) Both functions leak memory.

In #19, the possibility of memory leak in (b) was prevented. The problem of (c) still remains, but I have not come up with a solution method for now.

It is not assumed that a Ruby exception will occur inside call_without_gvl. So I think either FnMut or FnOnce can be used. If there is a possibility of an exception internally, we should wrap with vm::protect just like any other place.

[danielpclark]

Thanks @irxground .

I believe this PR is also following b) to avoid a premature free, from either Ruby or Rust, of memory that was still in use. So that's good and I appreciate you clarifying it.

---

As far as c) Resources created inside functions being a cause for c) Both functions leak memory I do think that this depends on which side of things a particular program is implemented towards (ie: a Ruby program using Rust vs a Rust program using Ruby). These are just my thoughts and I haven't proven it though.

If the program is a Rust extension to a Ruby program then I think resources created inside functions will be handled properly as Ruby has overall ownership of it.

As for Rust programs that integrate Ruby I think that may be more of a issue… and perhaps having a Rust wrapper object in Ruby be the resource creator from which the Ruby side needs to instantiate it. In other words create a custom struct wrapper object in Rust for Ruby to create with as a return type. This way when Ruby instantiates that object with input the ownership of what was created will reside within Rust. Also I haven't yet used wrapper struct objects from Rust in Ruby yet myself so I don't know from experience the benefits or disadvantages.

But I don't think that that's an issue this PR needs to be concerned with. :+1:

[dsander]

Thank you both!

[danielpclark]

No, thank you. :smiley: