Feature #20877
open
Introduce (public) debug assertion for holding the GVL.
Description
Background¶
I found issues with zlib.c calling rb_ functions without holding the GVL: https://bugs.ruby-lang.org/issues/20863. The GVL must be held before many of Ruby's C functions can be called. However, few functions check this. As such, bugs like this may exist in other parts of the code and native extensions.
Even thought it may work in many scenarios, it may break unexpectedly (SEGFAULT etc).
Proposal¶
Introduce more checks in CRuby¶
I think we should add more debug checks within CRuby for detecting this invalid usage.
The current internal implementation looks like this:
RUBY_ASSERT(ruby_thread_has_gvl_p());
While fixing zlib.c, I made a simple PR to demonstrate the change: https://github.com/ruby/ruby/pull/11975
Introduce a public macro for this check¶
I think we should expose a public macro that can be used by Ruby's native extensions so that they may do the same check.
ruby_thread_has_gvl_p is not a public interface at this time, so we can't use it in native extensions. I would like to consider adding a public macro to allow for this, e.g. RUBY_DEBUG_ASSERT_GVL or something similar.
Finally, I propose to add this macro to all methods that should be executed with the GVL so that we catch invalid usage.
If the name RUBY_DEBUG_ASSERT_GVL is not suitable / too specific, maybe some other ideas:
RUBY_DEBUG_ASSERT_THREAD-
RUBY_DEBUG_ASSERT_CURRENT_THREADorRUBY_DEBUG_ASSERT_CURRENT_THREAD_P -
RUBY_DEBUG_ASSERT_GVLorRUBY_DEBUG_ASSERT_GVL_P RUBY_DEBUG_ASSERT_INTERPRETER_LOCKED
I don't have a strong opinion on naming, but I have a strong opinion about preventing invalid usage.
Updated by