refactor: move inflight counter from CTimer to SignalInflight module

r1viollet · r1viollet · commit 1e751027b330 · 2026-06-29T17:58:15.000+02:00
Preparation for extending the JFR-teardown drain to all signal handlers
that write JFR (jbachorik review). The counter and drain logic were tied
to CTimer, but the question 'can JFR be torn down right now?' is a single
global question that any JFR-writing signal handler must answer. Tying it
to CTimer made it awkward to extend.

No behavior change. Pure rename/move:

- new signalInflight.h / signalInflight.cpp: SignalInflight class owns the
  cache-line-aligned counter, enter()/exit()/hasInflight()/drain() static
  methods, and the InflightGuard RAII type
- ctimer.h: drop _inflight, enterHandler, exitHandler, hasInflightHandlers,
  drainInflight, and the embedded InflightGuard class
- ctimer_linux.cpp: drop the counter definition and drainInflight()
  implementation
- profiler.cpp: Profiler::stop() now calls SignalInflight::drain() instead
  of CTimer::drainInflight()

Existing CTimer signal handlers still construct InflightGuard at entry;
that type now lives in signalInflight.h and the existing include of
ctimer.h is enough for them (ctimer_linux.cpp also includes signalInflight.h
explicitly for clarity).

Follow-up commit (B2) will add InflightGuard to PerfEvents, ITimer,
ITimerJvmti, WallClockASGCT, WallClockJvmti so they participate in the
drain too.
diff --git a/ddprof-lib/src/main/cpp/ctimer.h b/ddprof-lib/src/main/cpp/ctimer.h
@@ -26,13 +26,6 @@
 
 class CTimer : public Engine {
 private:
-  // Count of signal handlers currently executing past the _enabled check.
-  // drainInflight() waits for this to reach zero before JFR teardown, closing
-  // the TOCTOU window between the _enabled check and JFR buffer access.
-  // Placed on its own cache line to avoid false sharing with _enabled:
-  // _enabled is read-only on the hot path; _inflight is read-write.
-  alignas(64) static int _inflight;
-
   // cppcheck-suppress unusedPrivateFunction
   static void signalHandler(int signo, siginfo_t *siginfo, void *ucontext);
 
@@ -66,50 +59,10 @@ class CTimer : public Engine {
     __atomic_store_n(&_enabled, enabled, __ATOMIC_RELEASE);
   }
 
-  // In-flight handler tracking accessors (used by InflightGuard).
-  // ACQUIRE on increment / RELEASE on decrement so drainInflight() observes
-  // all handler-side writes before observing the counter at zero.
-  static void enterHandler() {
-    __atomic_fetch_add(&_inflight, 1, __ATOMIC_ACQUIRE);
-  }
-  static void exitHandler() {
-    __atomic_fetch_sub(&_inflight, 1, __ATOMIC_RELEASE);
-  }
-  static bool hasInflightHandlers() {
-    return __atomic_load_n(&_inflight, __ATOMIC_ACQUIRE) > 0;
-  }
-
-  // Spin until all signal handlers that passed the _enabled=true check have
-  // returned. Must be called with _enabled already false (after disableEngines()),
-  // before any JFR teardown that handlers could race against.
-  // Returns true if all handlers drained successfully, false if timeout fired.
-  // Caller must NOT proceed with JFR teardown if this returns false.
-  static bool drainInflight();
-
   // Get the signal number used by CTimer (0 if not initialized)
   static int getSignal() { return _signal; }
 };
 
-// RAII guard for CTimer signal handler in-flight tracking.
-// Increments the in-flight counter on construction and decrements on destruction,
-// ensuring the counter is always balanced regardless of which exit path the
-// handler takes. The counter (CTimer::_inflight) is cache-line-aligned to avoid
-// false sharing with _enabled, minimizing cache line bouncing.
-//
-// Usage (at the start of CTimer signal handlers):
-//   InflightGuard inflight;
-class InflightGuard {
-public:
-    InflightGuard()  { CTimer::enterHandler(); }
-    ~InflightGuard() { CTimer::exitHandler(); }
-
-    // Non-copyable, non-movable
-    InflightGuard(const InflightGuard&) = delete;
-    InflightGuard& operator=(const InflightGuard&) = delete;
-    InflightGuard(InflightGuard&&) = delete;
-    InflightGuard& operator=(InflightGuard&&) = delete;
-};
-
 // A CPU-time engine that reuses CTimer's per-thread timer_create / SIGPROF
 // dispatch, but instead of walking the stack in the signal handler delegates
 // the walk to HotSpot's JFR RequestStackTrace JVMTI extension. The sampled
@@ -141,19 +94,6 @@ class CTimer : public Engine {
   }
 
   static bool supported() { return false; }
-
-  // No-op on non-Linux platforms
-  static bool drainInflight() { return true; }
-  static bool hasInflightHandlers() { return false; }
-};
-
-// No-op InflightGuard on non-Linux platforms where CTimer is unavailable.
-class InflightGuard {
-public:
-    InflightGuard()  {}
-    ~InflightGuard() {}
-    InflightGuard(const InflightGuard&) = delete;
-    InflightGuard& operator=(const InflightGuard&) = delete;
 };
 
 class CTimerJvmti : public Engine {
diff --git a/ddprof-lib/src/main/cpp/ctimer_linux.cpp b/ddprof-lib/src/main/cpp/ctimer_linux.cpp
@@ -20,6 +20,7 @@
 #include "counters.h"
 #include "guards.h"
 #include "ctimer.h"
+#include "signalInflight.h"
 #include "debugSupport.h"
 #include "jvmThread.h"
 #include "libraries.h"
@@ -49,7 +50,6 @@ int CTimer::_max_timers = 0;
 int *CTimer::_timers = NULL;
 CStack CTimer::_cstack;
 bool CTimer::_enabled = false;
-alignas(64) int CTimer::_inflight = 0;
 int CTimer::_signal;
 
 int CTimer::registerThread(int tid) {
@@ -177,57 +177,12 @@ Error CTimer::start(Arguments &args) {
   return Error::OK;
 }
 
-// 200 ms: long enough for any legitimate recordSample() call to finish,
-// short enough to avoid a perceptible hang if a handler is somehow stuck.
-static const long DRAIN_TIMEOUT_NS = 200000000L;
-
-bool CTimer::drainInflight() {
-  if (__atomic_load_n(&_inflight, __ATOMIC_ACQUIRE) == 0) {
-    return true; // fast path: nothing in flight
-  }
-
-  struct timespec deadline;
-  if (clock_gettime(CLOCK_MONOTONIC, &deadline) != 0) {
-    // Cannot establish a deadline; refuse to spin unbounded and conservatively
-    // report timeout so the caller skips JFR teardown.
-    Log::error("CTimer::drainInflight: clock_gettime(CLOCK_MONOTONIC) failed (errno=%d). "
-               "Skipping JFR teardown to prevent use-after-free.", errno);
-    return false;
-  }
-  deadline.tv_nsec += DRAIN_TIMEOUT_NS;
-  if (deadline.tv_nsec >= 1000000000L) {
-    deadline.tv_sec += 1;
-    deadline.tv_nsec -= 1000000000L;
-  }
-
-  while (__atomic_load_n(&_inflight, __ATOMIC_ACQUIRE) > 0) {
-    struct timespec now;
-    if (clock_gettime(CLOCK_MONOTONIC, &now) != 0) {
-      Log::error("CTimer::drainInflight: clock_gettime(CLOCK_MONOTONIC) failed (errno=%d). "
-                 "Skipping JFR teardown to prevent use-after-free.", errno);
-      return false;
-    }
-    if (now.tv_sec > deadline.tv_sec ||
-        (now.tv_sec == deadline.tv_sec && now.tv_nsec >= deadline.tv_nsec)) {
-      int remaining = __atomic_load_n(&_inflight, __ATOMIC_ACQUIRE);
-      Log::error("CTimer::drainInflight: timed out after %ldms waiting for "
-                 "%d in-flight SIGPROF handler(s). Skipping JFR teardown to "
-                 "prevent use-after-free. This indicates a stuck signal handler.",
-                 DRAIN_TIMEOUT_NS / 1000000L, remaining);
-      return false; // timeout: handlers still in-flight
-    }
-    sched_yield();
-  }
-  return true; // success: all handlers drained
-}
-
 void CTimer::stop() {
   for (int i = 0; i < _max_timers; i++) {
     unregisterThread(i);
   }
-  // Note: drainInflight() is called by Profiler::stop() after all engines
-  // have stopped, not here. Profiler needs the return value to decide whether
-  // to proceed with JFR teardown.
+  // Note: SignalInflight::drain() is called by Profiler::stop() after
+  // disableEngines(); see signalInflight.h.
 }
 
 Error CTimerJvmti::check(Arguments &args) {
diff --git a/ddprof-lib/src/main/cpp/profiler.cpp b/ddprof-lib/src/main/cpp/profiler.cpp
@@ -14,6 +14,7 @@
 #include "common.h"
 #include "counters.h"
 #include "ctimer.h"
+#include "signalInflight.h"
 #include "dwarf.h"
 #include "flightRecorder.h"
 #include "itimer.h"
@@ -1504,19 +1505,19 @@ Error Profiler::stop() {
   }
 
   // Order matters: disable engines first so the _enabled check inside signal
-  // handlers will fail for any new signal delivered from now on. drainInflight()
-  // then waits for handlers that already passed the check to leave recordSample().
-  // Only once those are gone is it safe to run the rest of the teardown
+  // handlers will fail for any new signal delivered from now on. drain() then
+  // waits for handlers that already passed the check to leave their JFR write
+  // path. Only once those are gone is it safe to run the rest of the teardown
   // (engine stops, JFR teardown) without risking use-after-free.
   disableEngines();
 
-  if (!CTimer::drainInflight()) {
-    // SIGPROF handlers stuck past the 200 ms timeout. Leave state == RUNNING so
-    // the caller cannot start() against half-torn-down JFR and so engine stops
+  if (!SignalInflight::drain()) {
+    // Signal handlers stuck past the timeout. Leave state == RUNNING so the
+    // caller cannot start() against half-torn-down JFR and so engine stops
     // (notably BaseWallClock::stop()'s pthread_join) are not double-invoked.
     // The operation is idempotent on retry: disableEngines() above is an atomic
     // store, and no other engine stop has run yet.
-    return Error("SIGPROF handlers did not drain; teardown skipped, retry stop()");
+    return Error("signal handlers did not drain; teardown skipped, retry stop()");
   }
 
   if (_event_mask & EM_ALLOC)
diff --git a/ddprof-lib/src/main/cpp/signalInflight.cpp b/ddprof-lib/src/main/cpp/signalInflight.cpp
@@ -0,0 +1,66 @@
+/*
+ * Copyright 2026, Datadog, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "signalInflight.h"
+#include "log.h"
+
+#include <errno.h>
+#include <sched.h>
+#include <time.h>
+
+alignas(64) int SignalInflight::_counter = 0;
+
+// 200 ms: long enough for any legitimate signal handler to finish,
+// short enough to avoid a perceptible hang if a handler is somehow stuck.
+static const long DRAIN_TIMEOUT_NS = 200000000L;
+
+bool SignalInflight::drain() {
+  if (__atomic_load_n(&_counter, __ATOMIC_ACQUIRE) == 0) {
+    return true; // fast path: nothing in flight
+  }
+
+  struct timespec deadline;
+  if (clock_gettime(CLOCK_MONOTONIC, &deadline) != 0) {
+    Log::error("SignalInflight::drain: clock_gettime(CLOCK_MONOTONIC) failed (errno=%d). "
+               "Skipping JFR teardown to prevent use-after-free.", errno);
+    return false;
+  }
+  deadline.tv_nsec += DRAIN_TIMEOUT_NS;
+  if (deadline.tv_nsec >= 1000000000L) {
+    deadline.tv_sec += 1;
+    deadline.tv_nsec -= 1000000000L;
+  }
+
+  while (__atomic_load_n(&_counter, __ATOMIC_ACQUIRE) > 0) {
+    struct timespec now;
+    if (clock_gettime(CLOCK_MONOTONIC, &now) != 0) {
+      Log::error("SignalInflight::drain: clock_gettime(CLOCK_MONOTONIC) failed (errno=%d). "
+                 "Skipping JFR teardown to prevent use-after-free.", errno);
+      return false;
+    }
+    if (now.tv_sec > deadline.tv_sec ||
+        (now.tv_sec == deadline.tv_sec && now.tv_nsec >= deadline.tv_nsec)) {
+      int remaining = __atomic_load_n(&_counter, __ATOMIC_ACQUIRE);
+      Log::error("SignalInflight::drain: timed out after %ldms waiting for "
+                 "%d in-flight signal handler(s). Skipping JFR teardown to "
+                 "prevent use-after-free. This indicates a stuck signal handler.",
+                 DRAIN_TIMEOUT_NS / 1000000L, remaining);
+      return false;
+    }
+    sched_yield();
+  }
+  return true;
+}
diff --git a/ddprof-lib/src/main/cpp/signalInflight.h b/ddprof-lib/src/main/cpp/signalInflight.h
@@ -0,0 +1,69 @@
+/*
+ * Copyright 2026, Datadog, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef _SIGNAL_INFLIGHT_H
+#define _SIGNAL_INFLIGHT_H
+
+// Global tally of signal handlers currently executing past their _enabled
+// check and capable of writing JFR. Profiler::stop() waits (drain()) for the
+// counter to reach zero before calling _jfr.stop(), closing the TOCTOU
+// window between a handler's _enabled check and its JFR buffer access.
+//
+// All CPU and wall-clock signal handlers that can write JFR should construct
+// an InflightGuard at handler entry so they participate in the drain.
+//
+// The counter is on its own cache line (alignas(64)) so that handler-side
+// writes do not invalidate the cache line backing each engine's _enabled
+// flag, which is read on every signal.
+class SignalInflight {
+public:
+    // ACQUIRE on increment / RELEASE on decrement: drain() observes all
+    // handler-side writes before observing the counter at zero.
+    static void enter() {
+        __atomic_fetch_add(&_counter, 1, __ATOMIC_ACQUIRE);
+    }
+    static void exit() {
+        __atomic_fetch_sub(&_counter, 1, __ATOMIC_RELEASE);
+    }
+    static bool hasInflight() {
+        return __atomic_load_n(&_counter, __ATOMIC_ACQUIRE) > 0;
+    }
+
+    // Spin until the counter reaches zero or DRAIN_TIMEOUT_NS elapses.
+    // Returns true on clean drain, false on timeout. Callers MUST NOT proceed
+    // with JFR teardown when this returns false.
+    static bool drain();
+
+private:
+    alignas(64) static int _counter;
+};
+
+// RAII guard for signal-handler in-flight tracking. Construct as the first
+// statement (after SIGNAL_HANDLER_GUARD) of any signal handler that can
+// write JFR; the counter is incremented on construction and decremented on
+// destruction, balancing automatically across every return path.
+class InflightGuard {
+public:
+    InflightGuard()  { SignalInflight::enter(); }
+    ~InflightGuard() { SignalInflight::exit(); }
+
+    InflightGuard(const InflightGuard&)            = delete;
+    InflightGuard& operator=(const InflightGuard&) = delete;
+    InflightGuard(InflightGuard&&)                 = delete;
+    InflightGuard& operator=(InflightGuard&&)      = delete;
+};
+
+#endif // _SIGNAL_INFLIGHT_H
