[ruby-core:119783] [Ruby master Bug#20877] Introduce (public) debug assertion for holding the GVL.
Issue #20877 has been reported by ioquatix (Samuel Williams). ---------------------------------------- Bug #20877: Introduce (public) debug assertion for holding the GVL. https://bugs.ruby-lang.org/issues/20877 * Author: ioquatix (Samuel Williams) * Status: Open * Backport: 3.1: UNKNOWN, 3.2: UNKNOWN, 3.3: UNKNOWN ---------------------------------------- I found issues with `zlib.c` calling `rb_` functions without holding the GVL: https://bugs.ruby-lang.org/issues/20863 ## Background The GVL must be held before many of Ruby's C functions can be called. However, few functions enforce this. Even thought it may work in many scenarios, it may break unexpectedly (SEGFAULT etc). ## Proposal I think we should (1) add more debug checks within CRuby for detecting this invalid usage and (2) expose a public macro that can be used by Ruby's native extensions so that they may do the same. The current internal implementation looks like this: ```c 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 `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_THREAD` or `RUBY_DEBUG_ASSERT_CURRENT_THREAD_P` - `RUBY_DEBUG_ASSERT_GVL` or `RUBY_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. -- https://bugs.ruby-lang.org/
Issue #20877 has been updated by ioquatix (Samuel Williams). From the meeting notes: https://github.com/ruby/dev-meeting-log/blob/master/2024/DevMeeting-2024-11-...
matz: We can introduce internal checks RUBY_ASSERT(ruby_thread_has_gvl_p()); everywhere it makes sense. matz: We can expose ruby_thread_has_gvl_p() as a public interface include/ruby/thread.h
---------------------------------------- Feature #20877: Introduce (public) debug assertion for holding the GVL. https://bugs.ruby-lang.org/issues/20877#change-110716 * Author: ioquatix (Samuel Williams) * Status: Open ---------------------------------------- ## 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: ```c 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_THREAD` or `RUBY_DEBUG_ASSERT_CURRENT_THREAD_P` - `RUBY_DEBUG_ASSERT_GVL` or `RUBY_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. -- https://bugs.ruby-lang.org/
participants (1)
-
ioquatix (Samuel Williams)