8323425: JFR: Auto-generated filename doesn't work with time-limited …

…recording

Reviewed-by: mgronlun

src/jdk.jfr/share/classes/jdk/jfr/internal/PlatformRecording.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 2024, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -38,6 +38,8 @@
 import java.nio.file.StandardOpenOption;
 import java.security.AccessControlContext;
 import java.security.AccessController;
+import java.security.PrivilegedActionException;
+import java.security.PrivilegedExceptionAction;
 import java.time.Duration;
 import java.time.Instant;
 import java.time.LocalDateTime;
@@ -76,7 +78,7 @@ public final class PlatformRecording implements AutoCloseable {
     private boolean toDisk = true;
     private String name;
     private boolean dumpOnExit;
-    private SafePath dumpOnExitDirectory = new SafePath(".");
+    private SafePath dumpDirectory;
     // Timestamp information
     private Instant stopTime;
     private Instant startTime;
@@ -89,7 +91,7 @@ public final class PlatformRecording implements AutoCloseable {
     private TimerTask stopTask;
     private TimerTask startTask;
     @SuppressWarnings("removal")
-    private AccessControlContext noDestinationDumpOnExitAccessControlContext;
+    private final AccessControlContext dumpDirectoryControlContext;
     private boolean shouldWriteActiveRecordingEvent = true;
     private Duration flushInterval = Duration.ofSeconds(1);
     private long finalStartChunkNanos = Long.MIN_VALUE;
@@ -99,11 +101,11 @@ public final class PlatformRecording implements AutoCloseable {
     PlatformRecording(PlatformRecorder recorder, long id) {
         // Typically the access control context is taken
         // when you call dump(Path) or setDestination(Path),
-        // but if no destination is set and dumpOnExit=true
+        // but if no destination is set and the filename is auto-generated,
         // the control context of the recording is taken when the
         // Recording object is constructed. This works well for
         // -XX:StartFlightRecording and JFR.dump
-        this.noDestinationDumpOnExitAccessControlContext = AccessController.getContext();
+        this.dumpDirectoryControlContext = AccessController.getContext();
         this.id = id;
         this.recorder = recorder;
         this.name = String.valueOf(id);
@@ -174,7 +176,9 @@ public boolean stop(String reason) {
             newState = getState();
         }
         WriteableUserPath dest = getDestination();
-
+        if (dest == null && dumpDirectory != null) {
+            dest = makeDumpPath();
+        }
         if (dest != null) {
             try {
                 dumpStopped(dest);
@@ -191,6 +195,33 @@ public boolean stop(String reason) {
         return true;
     }
 
+    @SuppressWarnings("removal")
+    public WriteableUserPath makeDumpPath() {
+        try {
+            String name = JVMSupport.makeFilename(getRecording());
+            return AccessController.doPrivileged(new PrivilegedExceptionAction<WriteableUserPath>() {
+                @Override
+                public WriteableUserPath run() throws Exception {
+                    SafePath p = dumpDirectory;
+                    if (p == null) {
+                        p = new SafePath(".");
+                    }
+                    return new WriteableUserPath(p.toPath().resolve(name));
+                }
+            }, dumpDirectoryControlContext);
+        } catch (PrivilegedActionException e) {
+            Throwable t = e.getCause();
+            if (t instanceof SecurityException) {
+                Logger.log(LogTag.JFR, LogLevel.WARN, "Not allowed to create dump path for recording " + recording.getId() + " on exit.");
+            }
+            if (t instanceof IOException) {
+                Logger.log(LogTag.JFR, LogLevel.WARN, "Could not dump " + recording.getId() + " on exit.");
+            }
+            return null;
+        }
+    }
+
+
     public void scheduleStart(Duration delay) {
         synchronized (recorder) {
             ensureOkForSchedule();
@@ -697,11 +728,6 @@ void clearDestination() {
         destination = null;
     }
 
-    @SuppressWarnings("removal")
-    public AccessControlContext getNoDestinationDumpOnExitAccessControlContext() {
-        return noDestinationDumpOnExitAccessControlContext;
-    }
-
     void setShouldWriteActiveRecordingEvent(boolean shouldWrite) {
         this.shouldWriteActiveRecordingEvent = shouldWrite;
     }
@@ -828,12 +854,13 @@ private static List<RepositoryChunk> reduceFromEnd(Long maxSize, List<Repository
         return result;
     }
 
-    public void setDumpOnExitDirectory(SafePath directory) {
-       this.dumpOnExitDirectory = directory;
-    }
-
-    public SafePath getDumpOnExitDirectory()  {
-        return this.dumpOnExitDirectory;
+    /**
+     * Sets the dump directory.
+     * <p>
+     * Only to be used by DCmdStart.
+     */
+    public void setDumpDirectory(SafePath directory) {
+       this.dumpDirectory = directory;
     }
 
     public void setFlushInterval(Duration interval) {

src/jdk.jfr/share/classes/jdk/jfr/internal/ShutdownHook.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 2024, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -65,7 +65,7 @@ private void dump(PlatformRecording recording) {
         try {
             WriteableUserPath dest = recording.getDestination();
             if (dest == null) {
-                dest = makeDumpOnExitPath(recording);
+                dest = recording.makeDumpPath();
                 recording.setDestination(dest);
             }
             if (dest != null) {
@@ -78,29 +78,6 @@ private void dump(PlatformRecording recording) {
         }
     }
 
-    @SuppressWarnings("removal")
-    private WriteableUserPath makeDumpOnExitPath(PlatformRecording recording) {
-        try {
-            String name = JVMSupport.makeFilename(recording.getRecording());
-            AccessControlContext acc = recording.getNoDestinationDumpOnExitAccessControlContext();
-            return AccessController.doPrivileged(new PrivilegedExceptionAction<WriteableUserPath>() {
-                @Override
-                public WriteableUserPath run() throws Exception {
-                    return new WriteableUserPath(recording.getDumpOnExitDirectory().toPath().resolve(name));
-                }
-            }, acc);
-        } catch (PrivilegedActionException e) {
-            Throwable t = e.getCause();
-            if (t instanceof SecurityException) {
-                Logger.log(LogTag.JFR, LogLevel.WARN, "Not allowed to create dump path for recording " + recording.getId() + " on exit.");
-            }
-            if (t instanceof IOException) {
-                Logger.log(LogTag.JFR, LogLevel.WARN, "Could not dump " + recording.getId() + " on exit.");
-            }
-            return null;
-        }
-    }
-
     static final class ExceptionHandler implements Thread.UncaughtExceptionHandler {
         @Override
         public void uncaughtException(Thread t, Throwable e) {

src/jdk.jfr/share/classes/jdk/jfr/internal/dcmd/DCmdStart.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2012, 2024, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -29,6 +29,8 @@
 import java.nio.file.InvalidPathException;
 import java.nio.file.Path;
 import java.nio.file.Paths;
+import java.security.AccessControlContext;
+import java.security.AccessController;
 import java.text.ParseException;
 import java.time.Duration;
 import java.util.HashSet;
@@ -165,11 +167,12 @@ public void execute(ArgumentParser parser) throws DCmdException {
                     dumpOnExit = Boolean.TRUE;
                 }
                 Path p = Paths.get(path);
-                if (Files.isDirectory(p) && Boolean.TRUE.equals(dumpOnExit)) {
+                if (Files.isDirectory(p)) {
                     // Decide destination filename at dump time
                     // Purposely avoid generating filename in Recording#setDestination due to
                     // security concerns
-                    PrivateAccess.getInstance().getPlatformRecording(recording).setDumpOnExitDirectory(new SafePath(p));
+                    PlatformRecording pr = PrivateAccess.getInstance().getPlatformRecording(recording);
+                    pr.setDumpDirectory(new SafePath(p));
                 } else {
                     safePath = resolvePath(recording, path);
                     recording.setDestination(safePath.toPath());

test/jdk/jdk/jfr/jcmd/TestJcmdStartGeneratedFilename.java

@@ -0,0 +1,70 @@
+/*
+ * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+package jdk.jfr.jcmd;
+
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.concurrent.CountDownLatch;
+
+import jdk.jfr.FlightRecorder;
+import jdk.jfr.FlightRecorderListener;
+import jdk.jfr.Recording;
+import jdk.jfr.RecordingState;
+
+import jdk.test.lib.process.OutputAnalyzer;
+
+/**
+ * @test
+ * @summary Verify that a filename is generated
+ * @key jfr
+ * @requires vm.hasJFR
+ * @library /test/lib /test/jdk
+ * @run main/othervm jdk.jfr.jcmd.TestJcmdStartGeneratedFilename
+ */
+public class TestJcmdStartGeneratedFilename {
+
+    public static void main(String[] args) throws Exception {
+        CountDownLatch recordingClosed = new CountDownLatch(1);
+        FlightRecorder.addListener(new FlightRecorderListener() {
+            public void recordingStateChanged(Recording recording) {
+                if (recording.getState() == RecordingState.CLOSED) {
+                    recordingClosed.countDown();
+                }
+            }
+        });
+        Path directory = Paths.get(".", "recordings");
+        Files.createDirectories(directory);
+        JcmdHelper.jcmd("JFR.start", "duration=1s", "filename=" + directory);
+        recordingClosed.await();
+        for (Path path : Files.list(directory).toList()) {
+            String file = path.toString();
+            System.out.println("Found file: " + file);
+            if (file.endsWith(".jfr") && file.contains("hotspot-")) {
+                return;
+            }
+        }
+        throw new Exception("Expected dump file on the format hotspot-...jfr");
+    }
+}