Feature #20877: Introduce (public) debug assertion for holding the GVL.

• Status: Open
• Assignee: -
• Target version: -

[ioquatix (Samuel Williams)]

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_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.

[ioquatix (Samuel Williams)]

• Tracker changed from Bug to Feature
• Backport deleted (3.1: UNKNOWN, 3.2: UNKNOWN, 3.3: UNKNOWN)

[ioquatix (Samuel Williams)]

• Description updated (diff)