Issue #19172 has been updated by 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: ```c 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). ---------------------------------------- Bug #19172: `ruby_thread_has_gvl_p` is innacurate sometimes -- document or change? https://bugs.ruby-lang.org/issues/19172#change-119624 * Author: ivoanjo (Ivo Anjo) * Status: Assigned * Assignee: ko1 (Koichi Sasada) * ruby -v: (All Ruby versions) * Backport: 2.7: UNKNOWN, 3.0: UNKNOWN, 3.1: UNKNOWN ---------------------------------------- Howdy 👋! I work for Datadog [on the ddtrace gem](https://github.com/DataDog/dd-trace-rb) 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 ```c 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? -- https://bugs.ruby-lang.org/