[PROF-9342] Introduce profiler workaround for Ruby Dir interruption bug

benchmarks/profiler_hold_resume_interruptions.rb

@@ -0,0 +1,44 @@
+# Used to quickly run benchmark under RSpec as part of the usual test suite, to validate it didn't bitrot
+VALIDATE_BENCHMARK_MODE = ENV['VALIDATE_BENCHMARK'] == 'true'
+
+return unless __FILE__ == $PROGRAM_NAME || VALIDATE_BENCHMARK_MODE
+
+require 'benchmark/ips'
+require 'datadog'
+require 'pry'
+require_relative 'dogstatsd_reporter'
+
+# This benchmark measures the performance of the hold/resume interruptions used by the DirMonkeyPatches
+class ProfilerHoldResumeInterruptions
+  def create_profiler
+    Datadog.configure do |c|
+      c.profiling.enabled = true
+    end
+    Datadog::Profiling.wait_until_running
+  end
+
+  def run_benchmark
+    Benchmark.ips do |x|
+      benchmark_time = VALIDATE_BENCHMARK_MODE ? { time: 0.01, warmup: 0 } : { time: 10, warmup: 2 }
+      x.config(
+        **benchmark_time,
+        suite: report_to_dogstatsd_if_enabled_via_environment_variable(benchmark_name: 'profiler_hold_resume_interruptions')
+      )
+
+      x.report("hold / resume") do
+        Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_hold_interruptions
+        Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_resume_interruptions
+      end
+
+      x.save! 'profiler_hold_resume_interruptions-results.json' unless VALIDATE_BENCHMARK_MODE
+      x.compare!
+    end
+  end
+end
+
+puts "Current pid is #{Process.pid}"
+
+ProfilerHoldResumeInterruptions.new.instance_exec do
+  create_profiler
+  run_benchmark
+end

ext/datadog_profiling_native_extension/collectors_cpu_and_wall_time_worker.c

@@ -222,6 +222,8 @@ static VALUE _native_with_blocked_sigprof(DDTRACE_UNUSED VALUE self);
 static VALUE rescued_sample_allocation(VALUE tracepoint_data);
 static void delayed_error(struct cpu_and_wall_time_worker_state *state, const char *error);
 static VALUE _native_delayed_error(DDTRACE_UNUSED VALUE self, VALUE instance, VALUE error_msg);
+static VALUE _native_hold_interruptions(DDTRACE_UNUSED VALUE self);
+static VALUE _native_resume_interruptions(DDTRACE_UNUSED VALUE self);
 
 // Note on sampler global state safety:
 //
@@ -285,7 +287,9 @@ void collectors_cpu_and_wall_time_worker_init(VALUE profiling_module) {
   rb_define_singleton_method(collectors_cpu_and_wall_time_worker_class, "_native_allocation_count", _native_allocation_count, 0);
   rb_define_singleton_method(collectors_cpu_and_wall_time_worker_class, "_native_is_running?", _native_is_running, 1);
   rb_define_singleton_method(testing_module, "_native_current_sigprof_signal_handler", _native_current_sigprof_signal_handler, 0);
-  // TODO: Remove `_native_is_running` from `testing_module` once `prof-correctness` has been updated to not need it
+  rb_define_singleton_method(collectors_cpu_and_wall_time_worker_class, "_native_hold_interruptions", _native_hold_interruptions, 0);
+  rb_define_singleton_method(collectors_cpu_and_wall_time_worker_class, "_native_resume_interruptions", _native_resume_interruptions, 0);
+  // TODO: Remove `_native_is_running` from `testing_module` (should be in class) once `prof-correctness` has been updated to not need it
   rb_define_singleton_method(testing_module, "_native_is_running?", _native_is_running, 1);
   rb_define_singleton_method(testing_module, "_native_install_testing_signal_handler", _native_install_testing_signal_handler, 0);
   rb_define_singleton_method(testing_module, "_native_remove_testing_signal_handler", _native_remove_testing_signal_handler, 0);
@@ -1159,3 +1163,17 @@ static VALUE _native_delayed_error(DDTRACE_UNUSED VALUE self, VALUE instance, VA
 
   return Qnil;
 }
+
+// Masks SIGPROF interruptions for the current thread. Please don't use this -- you may end up with incomplete
+// profiling data.
+static VALUE _native_hold_interruptions(DDTRACE_UNUSED VALUE self) {
+  block_sigprof_signal_handler_from_running_in_current_thread();
+  return Qtrue;
+}
+
+// Unmasks SIGPROF interruptions for the current thread. If there's a pending sample, it'll be triggered inside this
+// method.
+static VALUE _native_resume_interruptions(DDTRACE_UNUSED VALUE self) {
+  unblock_sigprof_signal_handler_from_running_in_current_thread();
+  return Qtrue;
+}

ext/datadog_profiling_native_extension/setup_signal_handler.c

@@ -91,7 +91,7 @@ void remove_sigprof_signal_handler(void) {
   if (sigaction(SIGPROF, &signal_handler_config, NULL) != 0) rb_sys_fail("Failure while removing the signal handler");
 }
 
-static void toggle_sigprof_signal_handler_for_current_thread(int action) {
+static inline void toggle_sigprof_signal_handler_for_current_thread(int action) {
   sigset_t signals_to_toggle;
   sigemptyset(&signals_to_toggle);
   sigaddset(&signals_to_toggle, SIGPROF);

lib/datadog/core/configuration/settings.rb

@@ -397,6 +397,22 @@ def initialize(*_)
               end
             end
 
+            # The profiler gathers data by sending `SIGPROF` unix signals to Ruby application threads.
+            #
+            # We've discovered that this can trigger a bug in a number of Ruby APIs in the `Dir` class, as
+            # described in https://github.com/DataDog/dd-trace-rb/issues/3450 . This workaround prevents the issue
+            # from happening by monkey patching the affected APIs.
+            #
+            # (In the future, once a fix lands upstream, we'll disable this workaround for Rubies that don't need it)
+            #
+            # @default `DD_PROFILING_DIR_INTERRUPTION_WORKAROUND_ENABLED` environment variable as a boolean,
+            # otherwise `true`
+            option :dir_interruption_workaround_enabled do |o|
+              o.env 'DD_PROFILING_DIR_INTERRUPTION_WORKAROUND_ENABLED'
+              o.type :bool
+              o.default true
+            end
+
             # Configures how much wall-time overhead the profiler targets. The profiler will dynamically adjust the
             # interval between samples it takes so as to try and maintain the property that it spends no longer than
             # this amount of wall-clock time profiling. For example, with the default value of 2%, the profiler will

lib/datadog/profiling.rb

@@ -136,6 +136,7 @@ def self.allocation_count # rubocop:disable Lint/NestedMethodDefinition (On purp
       return false unless supported?
 
       require_relative 'profiling/ext/forking'
+      require_relative 'profiling/ext/dir_monkey_patches'
       require_relative 'profiling/collectors/info'
       require_relative 'profiling/collectors/code_provenance'
       require_relative 'profiling/collectors/cpu_and_wall_time_worker'

lib/datadog/profiling/component.rb

@@ -75,6 +75,10 @@ def self.build_profiler_component(settings:, agent_settings:, optional_tracer:)
         crashtracker = build_crashtracker(settings, transport)
         profiler = Profiling::Profiler.new(worker: worker, scheduler: scheduler, optional_crashtracker: crashtracker)
 
+        if dir_interruption_workaround_enabled?(settings, no_signals_workaround_enabled)
+          Datadog::Profiling::Ext::DirMonkeyPatches.apply!
+        end
+
         [profiler, { profiling_enabled: true }]
       end
 
@@ -445,6 +449,15 @@ def self.build_profiler_component(settings:, agent_settings:, optional_tracer:)
           libmysqlclient_version < Gem::Version.new('5.0.0') &&
           header_version >= Gem::Version.new('10.0.0'))
       end
+
+      private_class_method def self.dir_interruption_workaround_enabled?(settings, no_signals_workaround_enabled)
+        return false if no_signals_workaround_enabled
+
+        # NOTE: In the future this method will evolve to check for Ruby versions affected and not apply the workaround
+        # when it's not needed but currently all known Ruby versions are affected.
+
+        settings.profiling.advanced.dir_interruption_workaround_enabled
+      end
     end
   end
 end

lib/datadog/profiling/ext/dir_monkey_patches.rb

@@ -0,0 +1,223 @@
+# frozen_string_literal: true
+
+module Datadog
+  module Profiling
+    module Ext
+      # All Ruby versions as of this writing have bugs in the dir class implementation, causing issues such as
+      # https://github.com/DataDog/dd-trace-rb/issues/3450 .
+      #
+      # This monkey patch for the Ruby `Dir` class works around these bugs for affected Ruby versions by temporarily
+      # blocking the profiler from interrupting system calls.
+      #
+      # A lot of these APIs do very similar things -- they're provided by Ruby as helpers so users don't need to keep
+      # reimplementing them but share the same underlying buggy code. And so our monkey patches are a bit repetitive
+      # as well.
+      # We don't DRY out this file to have minimal overhead.
+      #
+      # These monkey patches are applied by the profiler when the "dir_interruption_workaround_enabled" setting is
+      # enabled. See the profiling settings for more detail.
+      module DirMonkeyPatches
+        def self.apply!
+          ::Dir.singleton_class.prepend(Datadog::Profiling::Ext::DirClassMonkeyPatches)
+          ::Dir.prepend(Datadog::Profiling::Ext::DirInstanceMonkeyPatches)
+
+          true
+        end
+      end
+
+      # Monkey patches for Dir.singleton_class. See DirMonkeyPatches above for more details.
+      module DirClassMonkeyPatches
+        def [](*args, &block)
+          Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_hold_interruptions
+          super
+        ensure
+          Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_resume_interruptions
+        end
+        ruby2_keywords :[] if respond_to?(:ruby2_keywords, true)
+
+        def children(*args, &block)
+          Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_hold_interruptions
+          super
+        ensure
+          Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_resume_interruptions
+        end
+        ruby2_keywords :children if respond_to?(:ruby2_keywords, true)
+
+        def each_child(*args, &block)
+          if block
+            begin
+              # <-- Begin critical region
+              Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_hold_interruptions
+              super do |entry_name|
+                Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_resume_interruptions
+                # <-- We're safe now while running customer code
+                yield entry_name
+                # <-- We'll go back to the Dir internals, critical region again
+                Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_hold_interruptions
+              end
+            ensure
+              # <-- End critical region
+              Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_resume_interruptions
+            end
+          else
+            # This returns an enumerator. We don't want/need to intercede here, the enumerator will eventually call the
+            # other branch once it gets going.
+            super
+          end
+        end
+        ruby2_keywords :each_child if respond_to?(:ruby2_keywords, true)
+
+        def empty?(*args, &block)
+          Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_hold_interruptions
+          super
+        ensure
+          Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_resume_interruptions
+        end
+        ruby2_keywords :empty? if respond_to?(:ruby2_keywords, true)
+
+        def entries(*args, &block)
+          Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_hold_interruptions
+          super
+        ensure
+          Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_resume_interruptions
+        end
+        ruby2_keywords :entries if respond_to?(:ruby2_keywords, true)
+
+        def foreach(*args, &block)
+          if block
+            begin
+              # <-- Begin critical region
+              Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_hold_interruptions
+              super do |entry_name|
+                Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_resume_interruptions
+                # <-- We're safe now while running customer code
+                yield entry_name
+                # <-- We'll go back to the Dir internals, critical region again
+                Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_hold_interruptions
+              end
+            ensure
+              # <-- End critical region
+              Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_resume_interruptions
+            end
+          else
+            # This returns an enumerator. We don't want/need to intercede here, the enumerator will eventually call the
+            # other branch once it gets going.
+            super
+          end
+        end
+        ruby2_keywords :foreach if respond_to?(:ruby2_keywords, true)
+
+        def glob(*args, &block)
+          if block
+            begin
+              # <-- Begin critical region
+              Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_hold_interruptions
+              super do |entry_name|
+                Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_resume_interruptions
+                # <-- We're safe now while running customer code
+                yield entry_name
+                # <-- We'll go back to the Dir internals, critical region again
+                Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_hold_interruptions
+              end
+            ensure
+              # <-- End critical region
+              Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_resume_interruptions
+            end
+          else
+            begin
+              Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_hold_interruptions
+              super
+            ensure
+              Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_resume_interruptions
+            end
+          end
+        end
+        ruby2_keywords :glob if respond_to?(:ruby2_keywords, true)
+
+        def home(*args, &block)
+          Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_hold_interruptions
+          super
+        ensure
+          Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_resume_interruptions
+        end
+        ruby2_keywords :home if respond_to?(:ruby2_keywords, true)
+      end
+
+      # Monkey patches for Dir. See DirMonkeyPatches above for more details.
+      module DirInstanceMonkeyPatches
+        def each(*args, &block)
+          if block
+            begin
+              # <-- Begin critical region
+              Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_hold_interruptions
+              super do |entry_name|
+                Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_resume_interruptions
+                # <-- We're safe now while running customer code
+                yield entry_name
+                # <-- We'll go back to the Dir internals, critical region again
+                Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_hold_interruptions
+              end
+            ensure
+              Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_resume_interruptions # <-- End critical region
+            end
+          else
+            # This returns an enumerator. We don't want/need to intercede here, the enumerator will eventually call the
+            # other branch once it gets going.
+            super
+          end
+        end
+        ruby2_keywords :each if respond_to?(:ruby2_keywords, true)
+
+        unless RUBY_VERSION.start_with?('2.5.') # This is Ruby 2.6+
+          def each_child(*args, &block)
+            if block
+              begin
+                # <-- Begin critical region
+                Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_hold_interruptions
+                super do |entry_name|
+                  Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_resume_interruptions
+                  # <-- We're safe now while running customer code
+                  yield entry_name
+                  # <-- We'll go back to the Dir internals, critical region again
+                  Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_hold_interruptions
+                end
+              ensure
+                # <-- End critical region
+                Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_resume_interruptions
+              end
+            else
+              # This returns an enumerator. We don't want/need to intercede here, the enumerator will eventually call the
+              # other branch once it gets going.
+              super
+            end
+          end
+          ruby2_keywords :each_child if respond_to?(:ruby2_keywords, true)
+
+          def children(*args, &block)
+            Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_hold_interruptions
+            super
+          ensure
+            Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_resume_interruptions
+          end
+          ruby2_keywords :children if respond_to?(:ruby2_keywords, true)
+        end
+
+        def tell(*args, &block)
+          Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_hold_interruptions
+          super
+        ensure
+          Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_resume_interruptions
+        end
+        ruby2_keywords :tell if respond_to?(:ruby2_keywords, true)
+
+        def pos(*args, &block)
+          Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_hold_interruptions
+          super
+        ensure
+          Datadog::Profiling::Collectors::CpuAndWallTimeWorker._native_resume_interruptions
+        end
+        ruby2_keywords :pos if respond_to?(:ruby2_keywords, true)
+      end
+    end
+  end
+end

sig/datadog/profiling.rbs

@@ -6,6 +6,9 @@ module Datadog
     def self.allocation_count: () -> ::Integer?
     def self.enabled?: () -> bool
     def self.wait_until_running: () -> true
+
+    private
+
     def self.replace_noop_allocation_count: () -> void
     def self.native_library_compilation_skipped?: () -> ::String?
     def self.try_reading_skipped_reason_file: (?untyped file_api) -> ::String?

sig/datadog/profiling/collectors/cpu_and_wall_time_worker.rbs

@@ -44,6 +44,8 @@ module Datadog
         def self._native_is_running?: (CpuAndWallTimeWorker self_instance) -> bool
         def self._native_allocation_count: () -> ::Integer?
         def self._native_sampling_loop: (CpuAndWallTimeWorker self_instance) -> void
+        def self._native_hold_interruptions: () -> void
+        def self._native_resume_interruptions: () -> void
 
         def wait_until_running: (timeout_seconds: ::Integer?) -> true
       end