Skip to content

Commit

Permalink
Raise ArgumentError when trying to use GVL profiling on unsupported R…
Browse files Browse the repository at this point in the history
…ubies
  • Loading branch information
ivoanjo committed Sep 20, 2024
1 parent 82aa51a commit c443893
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -802,8 +802,8 @@ static VALUE release_gvl_and_run_sampling_trigger_loop(VALUE instance) {
;
}

#ifndef NO_GVL_INSTRUMENTATION
if (state->gvl_profiling_enabled) {
if (state->gvl_profiling_enabled) {
#ifndef NO_GVL_INSTRUMENTATION
state->gvl_profiling_hook = rb_internal_thread_add_event_hook(
on_gvl_event,
(
Expand All @@ -814,8 +814,10 @@ static VALUE release_gvl_and_run_sampling_trigger_loop(VALUE instance) {
),
NULL
);
}
#endif
#else
rb_raise(rb_eArgError, "GVL profiling is not supported in this Ruby version");
#endif
}

// Flag the profiler as running before we release the GVL, in case anyone's waiting to know about it
rb_funcall(instance, rb_intern("signal_running"), 0);
Expand Down
20 changes: 16 additions & 4 deletions spec/datadog/profiling/collectors/cpu_and_wall_time_worker_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,18 @@
end
end

context "when gvl_profiling_enabled is true on an unsupported Ruby" do
before { skip "Behavior does not apply to current Ruby version" if RUBY_VERSION >= "3.3." }

let(:gvl_profiling_enabled) { true }

it do
expect(Datadog.logger).to receive(:warn).with(/GVL profiling is not supported/)

cpu_and_wall_time_worker.start
end
end

context "when gvl_profiling_enabled is false" do
let(:gvl_profiling_enabled) { false }

Expand Down Expand Up @@ -935,6 +947,8 @@

context "after starting" do
before do
skip_if_gvl_profiling_not_supported(self) if gvl_profiling_enabled

cpu_and_wall_time_worker.start
wait_until_running
end
Expand Down Expand Up @@ -967,8 +981,6 @@
end

context "when GVL profiling is enabled" do
before { skip_if_gvl_profiling_not_supported(self) { stop } }

let(:gvl_profiling_enabled) { true }

it "disables the GVL profiling hook" do
Expand Down Expand Up @@ -1005,6 +1017,8 @@
let(:options) { {thread_context_collector: thread_context_collector} }

before do
skip_if_gvl_profiling_not_supported(self) if gvl_profiling_enabled

# This is important -- the real #reset_after_fork must not be called concurrently with the worker running,
# which we do in this spec to make it easier to test the reset_after_fork behavior
allow(thread_context_collector).to receive(:reset_after_fork)
Expand All @@ -1024,8 +1038,6 @@
end

context "when gvl_profiling_enabled is true" do
before { skip_if_gvl_profiling_not_supported(self) }

let(:gvl_profiling_enabled) { true }

it "disables the gvl profiling hook" do
Expand Down
3 changes: 1 addition & 2 deletions spec/datadog/profiling/spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -122,9 +122,8 @@ def self.maybe_fix_label_range(key, value)
end
end

def skip_if_gvl_profiling_not_supported(testcase, &skip_steps)
def skip_if_gvl_profiling_not_supported(testcase)
if RUBY_VERSION < "3.3."
yield if skip_steps
testcase.skip "GVL profiling is only supported on Ruby >= 3.3"
end
end
Expand Down

0 comments on commit c443893

Please sign in to comment.