Clean up + add signal handler deregistration

DataDog · Apr 23, 2024 · 46805c1 · 46805c1
1 parent bb2ba28
commit 46805c1
Show file tree

Hide file tree

Showing 6 changed files with 42 additions and 17 deletions.
diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Linux/ProfilerSignalManager.cpp b/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Linux/ProfilerSignalManager.cpp
@@ -40,7 +40,7 @@ ProfilerSignalManager* ProfilerSignalManager::Get(int signal)
         throw std::invalid_argument(std::string("Signal argument is invalid ") + "(" + std::to_string(signal) + "). Value must be: 1 <= signal <= 31");
     }
 
-    auto* manager = &signalManagers[signal];
+    auto* manager = &signalManagers[signal - 1]; // 0-based array
     manager->SetSignal(signal);
 
     return manager;
@@ -73,6 +73,24 @@ bool ProfilerSignalManager::RegisterHandler(HandlerFn_t handler)
     return _isHandlerInPlace;
 }
 
+bool ProfilerSignalManager::UnRegisterHandler()
+{
+    if (!IsProfilerSignalHandlerInstalled())
+    {
+        return false;
+    }
+
+    int32_t result = sigaction(_signalToSend, &_previousAction, nullptr);
+    if (result != 0)
+    {
+        Log::Error("ProfilerSignalManager::UnRegisterHandler: Failed to un-register signal handler for ", _signalToSend, " signals. Reason: ",
+                   strerror(errno), ".");
+        return false;
+    }
+
+    return true;
+}
+
 void ProfilerSignalManager::SetSignal(int32_t signal)
 {
     _signalToSend = signal;

diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Linux/ProfilerSignalManager.h b/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Linux/ProfilerSignalManager.h
@@ -17,6 +17,7 @@ class ProfilerSignalManager
     static ProfilerSignalManager* Get(int signal);
 
     bool RegisterHandler(HandlerFn_t handler);
+    bool UnRegisterHandler();
     int32_t SendSignal(pid_t threadId);
     bool CheckSignalHandler();
     bool IsHandlerInPlace() const;

diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Linux/TimerCreateCpuProfiler.cpp b/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Linux/TimerCreateCpuProfiler.cpp
@@ -28,7 +28,6 @@ TimerCreateCpuProfiler::TimerCreateCpuProfiler(
     _pManagedThreadsList{pManagedThreadsList},
     _pProvider{pProvider},
     _callstackProvider{std::move(callstackProvider)},
-    _processId{OpSysTools::GetProcId()},
     _serviceState{ServiceState::Initialized},
     _samplingInterval{pConfiguration->GetCpuProfilingInterval()}
 {
@@ -94,6 +93,7 @@ void TimerCreateCpuProfiler::UnregisterThread(std::shared_ptr<ManagedThreadInfo>
 {
     if (_serviceState != ServiceState::Started)
     {
+        Log::Debug("timer_create-based cpu profiler is not started. Cannot unregister thread.");
         return;
     }
 
@@ -112,7 +112,7 @@ bool TimerCreateCpuProfiler::Start()
 {
     if (_serviceState.exchange(ServiceState::Started) == ServiceState::Started)
     {
-        // Log to say that it's already stated
+        Log::Debug("timer_create-based Cpu profiler has already been started.");
         return true;
     }
 
@@ -148,30 +148,27 @@ bool TimerCreateCpuProfiler::Stop()
     auto old = _serviceState.exchange(ServiceState::Stopped);
     if (old == ServiceState::Initialized)
     {
-        // TODO Log must be started first
+        Log::Debug("Cannot call Stop on the timer_create-based Cpu profiler since it was not started yet.");
         return false;
     }
 
     if (old == ServiceState::Stopped)
     {
-        // Maybe a race. Log to say that it's already stopped
+        Log::Debug("timer_create-based Cpu profiler was already stopped.");
         return true;
     }
 
-    // TODO
-    //_signalManager->UnRegisterHandler(SIGPROF);
+    _pSignalManager->UnRegisterHandler();
 
-    // for now it's ok not necessary to go through threads and delete the timer
-    // If Stop is called, the process is going down.
     return true;
 }
 
 bool TimerCreateCpuProfiler::CollectStackSampleSignalHandler(int sig, siginfo_t* info, void* ucontext)
 {
-    return Instance->Collect(info->si_pid, ucontext);
+    return Instance->Collect(ucontext);
 }
 
-bool TimerCreateCpuProfiler::Collect(pid_t callerProcess, void* ctx)
+bool TimerCreateCpuProfiler::Collect(void* ctx)
 {
     if (_serviceState == ServiceState::Stopped)
     {

diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Linux/TimerCreateCpuProfiler.h b/profiler/src/ProfilerEngine/Datadog.Profiler.Native.Linux/TimerCreateCpuProfiler.h
@@ -44,7 +44,7 @@ class TimerCreateCpuProfiler : public IService
     static bool CollectStackSampleSignalHandler(int sig, siginfo_t* info, void* ucontext);
     static TimerCreateCpuProfiler* Instance;
 
-    bool Collect(pid_t callerProcess, void* ucontext);
+    bool Collect(void* ucontext);
 
     enum class ServiceState
     {
@@ -57,7 +57,6 @@ class TimerCreateCpuProfiler : public IService
     IManagedThreadList* _pManagedThreadsList;
     CpuTimeProvider* _pProvider;
     CallstackProvider _callstackProvider;
-    pid_t _processId;
     std::atomic<ServiceState> _serviceState;
     std::chrono::milliseconds _samplingInterval;
 };
diff --git a/profiler/test/Datadog.Profiler.Native.Tests/CpuProfilerTypeTest.cpp b/profiler/test/Datadog.Profiler.Native.Tests/CpuProfilerTypeTest.cpp
@@ -36,7 +36,7 @@ TEST(CpuProfilerTypeTest, CheckManualCpuTimeCaseInsensitive)
 
 #ifdef LINUX
 
-TEST(CpuProfilerTypeTest, CheckTimeCreate)
+TEST(CpuProfilerTypeTest, CheckTimerCreate)
 {
     CpuProfilerType result;
     ASSERT_TRUE(convert_to(WStr("TimerCreate"), result));

diff --git a/profiler/test/Datadog.Profiler.Native.Tests/ProfilerSignalManagerTest.cpp b/profiler/test/Datadog.Profiler.Native.Tests/ProfilerSignalManagerTest.cpp
@@ -81,7 +81,7 @@ bool SigProfCustomHandler(int signal, siginfo_t* info, void* context)
     return true;
 }
 
-TEST_F(ProfilerSignalManagerFixture, CheckTwoDifferentSignalInstallation)
+TEST_F(ProfilerSignalManagerFixture, CheckTwoDifferentSignalsInstallation)
 {
     auto* sigusr1SignalManager = ProfilerSignalManager::Get(SIGUSR1);
     auto* sigprofSignalManager = ProfilerSignalManager::Get(SIGPROF);
@@ -101,7 +101,6 @@ TEST_F(ProfilerSignalManagerFixture, CheckTwoDifferentSignalInstallation)
     ASSERT_TRUE(SigProfHandlerCalled);
 }
 
-
 TEST_F(ProfilerSignalManagerFixture, CheckThrowIfSignalAbove31)
 {
     ASSERT_NO_THROW(ProfilerSignalManager::Get(SIGUSR1));
@@ -114,4 +113,15 @@ TEST_F(ProfilerSignalManagerFixture, CheckThrowIfSignalAbove31)
     ASSERT_THROW(ProfilerSignalManager::Get(100), std::invalid_argument);
 }
 
-#endif
+TEST_F(ProfilerSignalManagerFixture, CheckSignalDeRegistration)
+{
+    ProfilerSignalManager* manager = nullptr;
+    ASSERT_NO_THROW(manager = ProfilerSignalManager::Get(SIGPROF));
+
+    ASSERT_TRUE(manager->UnRegisterHandler());
+
+    // Make sure we do not un register more than once
+    ASSERT_FALSE(manager->UnRegisterHandler());
+}
+
+#endif