diff --git a/ext/datadog_profiling_native_extension/collectors_cpu_and_wall_time_worker.c b/ext/datadog_profiling_native_extension/collectors_cpu_and_wall_time_worker.c index 8ba916dd75..591354a906 100644 --- a/ext/datadog_profiling_native_extension/collectors_cpu_and_wall_time_worker.c +++ b/ext/datadog_profiling_native_extension/collectors_cpu_and_wall_time_worker.c @@ -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, ( @@ -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); diff --git a/spec/datadog/profiling/collectors/cpu_and_wall_time_worker_spec.rb b/spec/datadog/profiling/collectors/cpu_and_wall_time_worker_spec.rb index 7bf17c4f28..c39971c3f2 100644 --- a/spec/datadog/profiling/collectors/cpu_and_wall_time_worker_spec.rb +++ b/spec/datadog/profiling/collectors/cpu_and_wall_time_worker_spec.rb @@ -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 } @@ -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 @@ -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 @@ -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) @@ -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 diff --git a/spec/datadog/profiling/spec_helper.rb b/spec/datadog/profiling/spec_helper.rb index f04211f904..f813139111 100644 --- a/spec/datadog/profiling/spec_helper.rb +++ b/spec/datadog/profiling/spec_helper.rb @@ -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