improve error handling, fix start stop start flow

Author: lbloder

sentry-async-profiler/src/main/java/io/sentry/asyncprofiler/profiling/JavaContinuousProfiler.java

@@ -24,7 +24,6 @@
 import io.sentry.util.AutoClosableReentrantLock;
 import io.sentry.util.SentryRandom;
 import java.io.File;
-import java.io.IOException;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
@@ -52,13 +51,12 @@ public final class JavaContinuousProfiler
   private @Nullable Future<?> stopFuture;
   private final @NotNull List<ProfileChunk.Builder> payloadBuilders = new ArrayList<>();
   private @NotNull SentryId profilerId = SentryId.EMPTY_ID;
-  private @NotNull SentryId chunkId = SentryId.EMPTY_ID;
   private final @NotNull AtomicBoolean isClosed = new AtomicBoolean(false);
   private @NotNull SentryDate startProfileChunkTimestamp = new SentryNanotimeDate();
 
   private @NotNull String filename = "";
 
-  private final @NotNull AsyncProfiler profiler;
+  private @Nullable AsyncProfiler profiler;
   private volatile boolean shouldSample = true;
   private boolean shouldStop = false;
   private boolean isSampled = false;
@@ -76,21 +74,52 @@ public JavaContinuousProfiler(
     this.profilingTracesDirPath = profilingTracesDirPath;
     this.profilingTracesHz = profilingTracesHz;
     this.executorService = executorService;
-    this.profiler = AsyncProfiler.getInstance();
+    initializeProfiler();
+  }
+
+  private void initializeProfiler() {
+    try {
+      this.profiler = AsyncProfiler.getInstance();
+      // Check version to verify profiler is working
+      String version = profiler.execute("version");
+      logger.log(SentryLevel.DEBUG, "AsyncProfiler initialized successfully. Version: " + version);
+    } catch (Exception e) {
+      logger.log(
+          SentryLevel.WARNING,
+          "Failed to initialize AsyncProfiler. Profiling will be disabled.",
+          e);
+      this.profiler = null;
+    }
   }
 
   private boolean init() {
-    // We initialize it only once
     if (isInitialized) {
       return true;
     }
     isInitialized = true;
+
+    if (profiler == null) {
+      logger.log(SentryLevel.ERROR, "Disabling profiling because AsyncProfiler is not available.");
+      return false;
+    }
+
     if (profilingTracesDirPath == null) {
       logger.log(
           SentryLevel.WARNING,
           "Disabling profiling because no profiling traces dir path is defined in options.");
       return false;
     }
+
+    File profileDir = new File(profilingTracesDirPath);
+
+    if (!profileDir.canWrite() || !profileDir.exists()) {
+      logger.log(
+          SentryLevel.WARNING,
+          "Disabling profiling because traces directory is not writable or does not exist: %s",
+          profilingTracesDirPath);
+      return false;
+    }
+
     if (profilingTracesHz <= 0) {
       logger.log(
           SentryLevel.WARNING,
@@ -101,7 +130,6 @@ private boolean init() {
     return true;
   }
 
-  @SuppressWarnings("ReferenceEquality")
   @Override
   public void startProfiler(
       final @NotNull ProfileLifecycle profileLifecycle,
@@ -138,6 +166,7 @@ public void startProfiler(
       }
 
       if (!isRunning()) {
+        shouldStop = false;
         logger.log(SentryLevel.DEBUG, "Started Profiler.");
         start();
       }
@@ -159,7 +188,6 @@ private void initScopes() {
   private void start() {
     initScopes();
 
-    // Let's initialize trace folder and profiling interval
     if (!init()) {
       return;
     }
@@ -179,20 +207,31 @@ private void start() {
     } else {
       startProfileChunkTimestamp = new SentryNanotimeDate();
     }
+
+    if (profiler == null) {
+      logger.log(SentryLevel.ERROR, "Cannot start profiling: AsyncProfiler is not available");
+      return;
+    }
+
     filename = profilingTracesDirPath + File.separator + SentryUUID.generateSentryId() + ".jfr";
-    String startData = null;
+
+    File jfrFile = new File(filename);
+
     try {
       final String profilingIntervalMicros =
           String.format("%dus", (int) SECONDS.toMicros(1) / profilingTracesHz);
       final String command =
           String.format("start,jfr,wall=%s,file=%s", profilingIntervalMicros, filename);
-      System.out.println(command);
-      startData = profiler.execute(command);
+
+      profiler.execute(command);
+
     } catch (Exception e) {
       logger.log(SentryLevel.ERROR, "Failed to start profiling: ", e);
-    }
-    // check if profiling started
-    if (startData == null) {
+      filename = "";
+      // Try to clean up the file if it was created
+      if (jfrFile.exists()) {
+        jfrFile.delete();
+      }
       return;
     }
 
@@ -202,10 +241,6 @@ private void start() {
       profilerId = new SentryId();
     }
 
-    if (chunkId == SentryId.EMPTY_ID) {
-      chunkId = new SentryId();
-    }
-
     try {
       stopFuture = executorService.schedule(() -> stop(true), MAX_CHUNK_DURATION_MILLIS);
     } catch (RejectedExecutionException e) {
@@ -249,45 +284,57 @@ private void stop(final boolean restartProfiler) {
       // check if profiler was created and it's running
       if (!isRunning) {
         // When the profiler is stopped due to an error (e.g. offline or rate limited), reset the
-        // ids
+        // id
         profilerId = SentryId.EMPTY_ID;
-        chunkId = SentryId.EMPTY_ID;
         return;
       }
 
-      String endData = null;
+      File jfrFile = new File(filename);
+
+      if (profiler == null) {
+        logger.log(SentryLevel.WARNING, "Profiler is null when trying to stop");
+        return;
+      }
+
       try {
-        endData = profiler.execute("stop,jfr");
-      } catch (IOException e) {
-        throw new RuntimeException(e);
+        profiler.execute("stop,jfr");
+      } catch (Exception e) {
+        logger.log(SentryLevel.ERROR, "Error stopping profiler, attempting cleanup: ", e);
+        // Clean up file if it exists
+        if (jfrFile.exists()) {
+          jfrFile.delete();
+        }
       }
 
-      // check if profiler end successfully
-      if (endData == null) {
-        logger.log(
-            SentryLevel.ERROR,
-            "An error occurred while collecting a profile chunk, and it won't be sent.");
-      } else {
-        // The scopes can be null if the profiler is started before the SDK is initialized (app
-        // start profiling), meaning there's no scopes to send the chunks. In that case, we store
-        // the data in a list and send it when the next chunk is finished.
+      // The scopes can be null if the profiler is started before the SDK is initialized (app
+      // start profiling), meaning there's no scopes to send the chunks. In that case, we store
+      // the data in a list and send it when the next chunk is finished.
+      if (jfrFile.exists() && jfrFile.canRead() && jfrFile.length() > 0) {
         try (final @NotNull ISentryLifecycleToken ignored2 = payloadLock.acquire()) {
-          File jfrFile = new File(filename);
           jfrFile.deleteOnExit();
           payloadBuilders.add(
               new ProfileChunk.Builder(
                   profilerId,
-                  chunkId,
+                  new SentryId(),
                   new HashMap<>(),
                   jfrFile,
                   startProfileChunkTimestamp,
-                  ProfileChunk.PLATFORM_ANDROID));
+                  ProfileChunk.PLATFORM_JAVA));
+        }
+      } else {
+        logger.log(
+            SentryLevel.WARNING,
+            "JFR file is invalid or empty: exists=%b, readable=%b, size=%d",
+            jfrFile.exists(),
+            jfrFile.canRead(),
+            jfrFile.length());
+        if (jfrFile.exists()) {
+          jfrFile.delete();
         }
       }
 
+      // Always clean up state, even if stop failed
       isRunning = false;
-      // A chunk is finished. Next chunk will have a different id.
-      chunkId = SentryId.EMPTY_ID;
       filename = "";
 
       if (scopes != null) {
@@ -356,7 +403,9 @@ private void sendChunks(final @NotNull IScopes scopes, final @NotNull SentryOpti
 
   @Override
   public boolean isRunning() {
-    return isRunning;
+    try (final @NotNull ISentryLifecycleToken ignored = lock.acquire()) {
+      return isRunning && profiler != null && !filename.isEmpty();
+    }
   }
 
   @VisibleForTesting

sentry-async-profiler/src/test/java/io/sentry/asyncprofiler/profiling/JavaContinuousProfilerTest.kt

@@ -20,6 +20,7 @@ import kotlin.test.Test
 import kotlin.test.assertEquals
 import kotlin.test.assertFalse
 import kotlin.test.assertTrue
+import org.mockito.ArgumentMatchers.startsWith
 import org.mockito.Mockito
 import org.mockito.kotlin.any
 import org.mockito.kotlin.eq
@@ -274,7 +275,13 @@ class JavaContinuousProfilerTest {
     fixture.executor.runAll()
     // We assert that no trace files are written
     assertTrue(File(fixture.options.profilingTracesDirPath!!).list()!!.isEmpty())
-    verify(fixture.mockLogger).log(eq(SentryLevel.ERROR), eq("Failed to start profiling: "), any())
+    val expectedPath = fixture.options.profilingTracesDirPath
+    verify(fixture.mockLogger)
+      .log(
+        eq(SentryLevel.WARNING),
+        eq("Disabling profiling because traces directory is not writable or does not exist: %s"),
+        eq(expectedPath),
+      )
   }
 
   @Test
@@ -324,7 +331,6 @@ class JavaContinuousProfilerTest {
   @Test
   fun `close without terminating stops all profiles after chunk is finished`() {
     val profiler = fixture.getSut()
-    profiler.startProfiler(ProfileLifecycle.MANUAL, fixture.mockTracesSampler)
     profiler.startProfiler(ProfileLifecycle.TRACE, fixture.mockTracesSampler)
     assertTrue(profiler.isRunning)
     // We are scheduling the profiler to stop at the end of the chunk, so it should still be running
@@ -338,6 +344,24 @@ class JavaContinuousProfilerTest {
     assertFalse(profiler.isRunning)
   }
 
+  @Test
+  fun `profiler can be stopped and restarted`() {
+    val profiler = fixture.getSut()
+    profiler.startProfiler(ProfileLifecycle.MANUAL, fixture.mockTracesSampler)
+    assertTrue(profiler.isRunning)
+
+    profiler.stopProfiler(ProfileLifecycle.MANUAL)
+    fixture.executor.runAll()
+    assertFalse(profiler.isRunning)
+
+    profiler.startProfiler(ProfileLifecycle.MANUAL, fixture.mockTracesSampler)
+    fixture.executor.runAll()
+
+    assertTrue(profiler.isRunning)
+    verify(fixture.mockLogger, never())
+      .log(eq(SentryLevel.WARNING), startsWith("JFR file is invalid or empty"), any(), any(), any())
+  }
+
   @Test
   fun `profiler does not send chunks after close`() {
     val profiler = fixture.getSut()