review fixes

grafana · Apr 23, 2024 · 4fe4a95 · 4fe4a95
1 parent 96a56ae
commit 4fe4a95
Show file tree

Hide file tree

Showing 2 changed files with 18 additions and 5 deletions.
diff --git 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,17 +59,20 @@ public static void start(Options options) {
             } catch (final Throwable e) {
                 sFailed = true;
                 logger.log(Logger.Level.ERROR, "Error starting profiler %s", e);
+                sOptions = null;
             }
         }
     }
 
     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
@@ -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");
     }