From 4fe4a956110044d9ed6fea3b25c94da9760aaed5 Mon Sep 17 00:00:00 2001
From: Tolya Korniltsev <korniltsev.anatoly@gmail.com>
Date: Wed, 24 Apr 2024 00:26:31 +0700
Subject: [PATCH] review fixes

---
 .../java/io/pyroscope/javaagent/PyroscopeAgent.java  | 11 ++++++++++-
 .../javaagent/impl/ContinuousProfilingScheduler.java | 12 ++++++++----
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/agent/src/main/java/io/pyroscope/javaagent/PyroscopeAgent.java b/agent/src/main/java/io/pyroscope/javaagent/PyroscopeAgent.java
index 61bf2f9..88fb919 100644
--- a/agent/src/main/java/io/pyroscope/javaagent/PyroscopeAgent.java
+++ b/agent/src/main/java/io/pyroscope/javaagent/PyroscopeAgent.java
@@ -48,7 +48,7 @@ public static void start(Options options) {
                 return;
             }
             if (sFailed) {
-                logger.log(Logger.Level.INFO, "Not start profiling due to recent failure");
+                logger.log(Logger.Level.ERROR, "Not start profiling due to recent failure");
                 return;
             }
             sOptions = options;
@@ -59,6 +59,7 @@ public static void start(Options options) {
             } catch (final Throwable e) {
                 sFailed = true;
                 logger.log(Logger.Level.ERROR, "Error starting profiler %s", e);
+                sOptions = null;
             }
         }
     }
@@ -66,10 +67,12 @@ public static void start(Options options) {
     public static void stop() {
         synchronized (sLock) {
             if (sOptions == null) {
+                DefaultLogger.PRECONFIG_LOGGER.log(Logger.Level.ERROR, "Error stopping profiler: not started");
                 return;
             }
             try {
                 sOptions.scheduler.stop();
+                sOptions.logger.log(Logger.Level.INFO, "Profiling stopped");
             } catch (Throwable e) {
                 sOptions.logger.log(Logger.Level.ERROR, "Error stopping profiler %s", e);
                 sFailed = true;
@@ -79,6 +82,12 @@ public static void stop() {
         }
     }
 
+    public static boolean isStarted() {
+        synchronized (sLock) {
+            return sOptions != null;
+        }
+    }
+
     /**
      * Options allow to swap pyroscope components:
      * - io.pyroscope.javaagent.api.ProfilingScheduler
diff --git a/agent/src/main/java/io/pyroscope/javaagent/impl/ContinuousProfilingScheduler.java b/agent/src/main/java/io/pyroscope/javaagent/impl/ContinuousProfilingScheduler.java
index fe55db8..0e68b32 100644
--- a/agent/src/main/java/io/pyroscope/javaagent/impl/ContinuousProfilingScheduler.java
+++ b/agent/src/main/java/io/pyroscope/javaagent/impl/ContinuousProfilingScheduler.java
@@ -56,19 +56,23 @@ public void start(Profiler profiler) {
             this.job = executor.scheduleAtFixedRate(this::schedulerTick,
                 firstProfilingDuration.toMillis(), config.uploadInterval.toMillis(), TimeUnit.MILLISECONDS);
             this.started = true;
+            logger.log(Logger.Level.DEBUG, "ContinuousProfilingScheduler started");
         }
     }
 
     @Override
     public void stop() {
-        ScheduledExecutorService svc = this.executor;
+        ScheduledExecutorService svc;
         synchronized (lock) {
             stopSchedulerLocked();
+            svc = this.executor;
             this.executor = null;
         }
-        if (svc != null) {
-            awaitTermination(svc);
-        }
+        // shutdown here not under lock to avoid deadlock ( the task may block to wait for lock and
+        // we are holding the lock and waiting for task to finish)
+        // There is still synchronization happens from the PyroscopeAgent class,
+        // so there are no concurrent calls to start/stop. So there is no lock here
+        awaitTermination(svc);
         this.logger.log(Logger.Level.DEBUG, "ContinuousProfilingScheduler stopped");
     }