Name the async_cb_thread for easier debugging

[ivoanjo]

I found this useful while reviewing DataDog/dd-trace-rb#1371 -- we were searching for specs that leaked threads, and saw a "mysterious" thread with no stack that we couldn't exactly point out what created it or why it existed.

By setting a name on the callback thread, it becomes immediately obvious when looking at Thread.list what this thread is and where it comes from.

(Thread naming has been around since Ruby 2.3 which means we can do it safely for every supported version)

[larskanis]

To name the thread is a great idea! Two thoughts:

• It's MRI specific, so that JRuby and TruffleRuby needs to be excluded.
• I think it's useful to name the callback thread as well. Maybe as "FFI::Function Callback"?

[ivoanjo]

Thanks for the feedback!

It's MRI specific, so that JRuby and TruffleRuby needs to be excluded.

Yeap, already skipped both. It's weird that CI for JRuby seemed to pass even without my change, because locally I did see that it didn't work on JRuby.

I think it's useful to name the callback thread as well. Maybe as "FFI::Function Callback"?

Sounds good! I'll look into it -- I'm guessing it needs a bit of wrapping, because the Thread gets started with the callback directly, so I'll probably need to wrap the callback with the equivalent of a proc that first sets the name on the thread it runs on, and then goes on to run the callback.

[larskanis]

It's MRI specific, so that JRuby and TruffleRuby needs to be excluded.

Yeap, already skipped both. It's weird that CI for JRuby seemed to pass even without my change, because locally I did see that it didn't work on JRuby.

Unfortunately github actions doesn't support allow_failure so far, so that we use continue-on-error as a workaround.

I think it's useful to name the callback thread as well. Maybe as "FFI::Function Callback"?

Sounds good! I'll look into it -- I'm guessing it needs a bit of wrapping, because the Thread gets started with the callback directly, so I'll probably need to wrap the callback with the equivalent of a proc that first sets the name on the thread it runs on, and then goes on to run the callback.

I think it's OK to set the thread name from the thread calling rb_thread_create() through the result of this function. The same way as for the dispatcher. I didn't find any information about thread safety of Thread#name= but at least the corresponding pthread function is labled thread save and the sample program sets the name from the main thread not from the created thread.

[ivoanjo]

I think it's useful to name the callback thread as well. Maybe as "FFI::Function Callback"?

Done! I ended up using "Callback Runner" since it was a bit weird to have a "FFI::Function Callback" and a "FFI::Function Callback Dispatcher", but let me know if you still prefer your original suggestion :)

[ivoanjo]

Hmmm I guess I have some debugging to do on windows. I'm guessing it may be due to the racy manner in which we set the name in one thread and then access it in the other.

[larskanis]

It should be race-free to store Thread.current here and call Thread#name below. The name is readable even if the thread has terminated.

[ivoanjo]

https://github.com/ffi/ffi/blob/master/spec/ffi/fixtures/FunctionTest.c#L128
Hah! Nice troll!

I can try to name the thread all day long! The "async" callback is not async on windows xD

Took me a bit to debug this ahahaha :)

[ivoanjo]

Thanks for picking up the last bit @larskanis! I admit I was not having much sucess poking at the win32 bits and fighting with _beginthreadex -- I had gotten so far as to make it work on Ruby 2.7 and 2.6, but was getting segmentation faults on older Rubies.

🙇 thanks for the help!

[larskanis]

No problem. Thank you for contributing this!