Bug #19172: `ruby_thread_has_gvl_p` is innacurate sometimes -- document or change?

• Status: Assigned
• Assignee: ko1 (Koichi Sasada)
• Target version: -
• ruby -v: (All Ruby versions)
• Backport: 2.7: UNKNOWN, 3.0: UNKNOWN, 3.1: UNKNOWN

[ivoanjo (Ivo Anjo)]

Howdy 👋! I work for Datadog on the ddtrace gem and I found a... sharp edge on the internal ruby_thread_has_gvl_p API.

I am aware that ruby_thread_has_gvl_p is documented an experimental API that is exported as a symbol but not present on the VM include files.

Background

In the ddtrace profiling component, we setup a signal handler and then periodically send SIGPROF signals to try to interrupt the running Ruby thread (e.g. the thread that is holding the global VM lock or equivalent).

In the signal handler, we need to perform some API calls which are not safe to do without the GVL. So we need to check if the signal handler got called in the thread that has the GVL.

The issue

int
ruby_thread_has_gvl_p(void)
{
    rb_thread_t *th = ruby_thread_from_native();

    if (th && th->blocking_region_buffer == 0) {
        return 1;
    }
    else {
        return 0;
    }
}

In its current implementation, ruby_thread_has_gvl_p only checks if the thread has a blocking_region_buffer or not. Unfortunately, this means that when called from a thread that lost the GVL but not due to blocking (e.g. via rb_thread_schedule()), it can still claim that a thread is holding the GVL when that is not the case.

I ran into this issue in https://github.com/DataDog/dd-trace-rb/pull/2415, and needed to find a workaround.

Next steps

Since this is an internal VM API, I'm not sure you'd want to change the current behavior, so I was thinking of perhaps two options:

• Is it worth changing ruby_thread_has_gvl_p to be accurate in the case I've listed?

• If not, would you accept a PR to document its current limitations, so that others don't run into the same issue I did?

[ivoanjo (Ivo Anjo)]

• Description updated (diff)

[luke-gru (Luke Gruber)]

In doing some reading of the code, it's my understanding that (TH_SCHED(th)->running == th) would be a better way to tell if a thread has the GVL. Maybe I'm understanding it wrong though.

[ivoanjo (Ivo Anjo)]

Yeah, that's my understanding, and what I'm using in that PR (although with a lot more complexity since I'm still trying to support older Rubies...).

[Eregon (Benoit Daloze)]

The implementation is still the same as of today:
https://github.com/ruby/ruby/blob/4d87d43b01dbb312eb1ff5fbbc6c9f33218d91a2/thread.c#L2100-L2115

Meanwhile, ruby_thread_has_gvl_p() became a public API since #20877 (commit).

So now we have a public C API with known incorrect semantics when used in a signal handler, what should we do then?

[Eregon (Benoit Daloze)]

• Related to Feature #20877: Introduce (public) debug assertion for holding the GVL. added

[mame (Yusuke Endoh)]

• Status changed from Open to Assigned
• Assignee set to ko1 (Koichi Sasada)

[jhawthorn (John Hawthorn)]

Eregon (Benoit Daloze) wrote in #note-4:

So now we have a public C API with known incorrect semantics when used in a signal handler, what should we do then?

This public API is not documented as async-signal-safe, so there is no issue here. Calling ruby_thread_has_gvl_p() or almost any other function in this context is invalid. The list of stdlib functions which are valid to call in this context is tiny, and the same is true of Ruby: only rb_postponed_job_*, rb_internal_thread_specific_{get,set}, and rb_close_before_exec are async signal safe (I use rb_profile_frames in vernier/stackprof as if it's async-signal-safe, but it isn't officially and there's definitely been issues).

I don't think this is a bug and should be closed.

[Eregon (Benoit Daloze)]

I can see that point of view and I would agree for most functions, but not for ruby_thread_has_gvl_p() and not for ruby_native_thread_p().

Both of these functions, unlike almost all others, can be called without the GVL. If they couldn't they would be useless (because they'd always return true in valid usages then).
Their naming of ruby_ also seems indicative they are not the usual rb_*() functions which can only be called with the GVL held.

Because these 2 functions can be called without the GVL I believe there is a strong expectation that they can be called anywhere, including in signal handlers.
And besides expectations I think there is a need for internal profilers to call them in signal handlers:

In fact StackProf, Vernier and the Datadog profiler all call ruby_native_thread_p() in the SIGPROF handler, because it would be unsafe to rb_profile_frames() a non-Ruby thread.
They also call rb_during_gc(), because it's unsafe to rb_profile_frames() during GC, for example during GC compaction which installs a read barrier (we have seen crashes without that check).
Now we get to ruby_thread_has_gvl_p(), I think it's unsafe to call rb_profile_frames() while not holding the GVL, because for example the other thread holding the GVL might start a GC, including compaction, and rb_profile_frames() could fail while reading stuff from ISEQ & CME, notably to resolve lines, because objects become T_MOVED with GC compaction, or because of the GC compaction read barrier, etc.
That last one is reasoning-based, but for example if GC compaction protects a page while rb_profile_frames() reads state from ISEQ/CME, it might get in the GC compaction signal handler, which RB_GC_VM_LOCK() inside SIGSEGV, potentially holding thread_sched_lock too and it all feels very unsafe.

Hence I think the correct check is:

if (ruby_native_thread_p() && ruby_thread_has_gvl_p() && rb_during_gc()) {
  rb_profile_frames(0, MAX_LEN, frames, lines);
}

Also I will note that the implementation of ruby_thread_has_gvl_p() is async-signal-safe, but it returns the wrong value in a signal handler.
ruby_native_thread_p is async-signal-safe, as long as RB_THREAD_LOCAL_SPECIFIER is defined (pthread_getspecific() is not officially async-signal-safe though it is for many implementations in practice).
rb_during_gc looks async-signal-safe if called with the GVL. Unsure without the GVL as it's reading from a concurrently-mutated bitfield which is scary, and it's racy read as the GC might start concurrently. That's one of the main reasons I think we need the ruby_thread_has_gvl_p() check too.

All these 3 functions are currently not documented as async-signal-safe, but I think we should document them based on the above.
And fix ruby_thread_has_gvl_p() to return the correct value (the Datadog profiler currently uses internal headers to workaround ruby_thread_has_gvl_p() not returning the correct value).