From 004d0ecf8693961455f4fe75cf0232aa6eead307 Mon Sep 17 00:00:00 2001 From: Erik Gahlin Date: Wed, 29 Apr 2026 16:30:22 +0000 Subject: [PATCH] 8373439: Deadlock between flight recorder & VM shutdown Reviewed-by: mgronlun --- .../share/classes/jdk/jfr/EventSettings.java | 4 +- .../share/classes/jdk/jfr/FlightRecorder.java | 10 +- .../share/classes/jdk/jfr/Recording.java | 9 +- .../jdk/jfr/consumer/RecordingStream.java | 11 +- .../jdk/jfr/internal/PlatformRecorder.java | 58 ++++-- .../jdk/jfr/internal/PlatformRecording.java | 12 +- .../jdk/jfr/internal/PrivateAccess.java | 3 +- test/jdk/jdk/jfr/jvm/AfterShutdown.java | 190 ++++++++++++++++++ test/jdk/jdk/jfr/jvm/TestAfterShutdown.java | 44 ++++ 9 files changed, 310 insertions(+), 31 deletions(-) create mode 100644 test/jdk/jdk/jfr/jvm/AfterShutdown.java create mode 100644 test/jdk/jdk/jfr/jvm/TestAfterShutdown.java diff --git a/src/jdk.jfr/share/classes/jdk/jfr/EventSettings.java b/src/jdk.jfr/share/classes/jdk/jfr/EventSettings.java index c9f24d9f903..a8de9592140 100644 --- a/src/jdk.jfr/share/classes/jdk/jfr/EventSettings.java +++ b/src/jdk.jfr/share/classes/jdk/jfr/EventSettings.java @@ -151,8 +151,8 @@ public abstract class EventSettings { } @Override - public Recording newRecording(RecordingState state) { - return new Recording(state, Map.of()); + public Recording newRecording(Boolean register) { + return new Recording(register, Map.of()); } @Override diff --git a/src/jdk.jfr/share/classes/jdk/jfr/FlightRecorder.java b/src/jdk.jfr/share/classes/jdk/jfr/FlightRecorder.java index 6ea6f87f2af..1d6298be9bb 100644 --- a/src/jdk.jfr/share/classes/jdk/jfr/FlightRecorder.java +++ b/src/jdk.jfr/share/classes/jdk/jfr/FlightRecorder.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2016, 2025, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2016, 2026, 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 @@ -99,8 +99,12 @@ public final class FlightRecorder { */ public Recording takeSnapshot() { Recording snapshot = new Recording(); - snapshot.setName("Snapshot"); - internal.fillWithRecordedData(snapshot.getInternal(), null); + snapshot.getInternal().setName("Snapshot", false); + synchronized (internal) { + if (!internal.isDestroyed()) { + internal.fillWithRecordedData(snapshot.getInternal(), null); + } + } return snapshot; } diff --git a/src/jdk.jfr/share/classes/jdk/jfr/Recording.java b/src/jdk.jfr/share/classes/jdk/jfr/Recording.java index 8a3181307d2..466654180eb 100644 --- a/src/jdk.jfr/share/classes/jdk/jfr/Recording.java +++ b/src/jdk.jfr/share/classes/jdk/jfr/Recording.java @@ -35,7 +35,6 @@ import java.util.HashMap; import java.util.Map; import java.util.Objects; -import jdk.jfr.RecordingState; import jdk.jfr.internal.PlatformRecorder; import jdk.jfr.internal.PlatformRecording; import jdk.jfr.internal.Type; @@ -101,16 +100,16 @@ public final class Recording implements Closeable { * @since 11 */ public Recording(Map settings) { - this(RecordingState.NEW, settings); + this(null, settings); } // package private - Recording(RecordingState state, Map settings) { + Recording(Boolean register, Map settings) { Objects.requireNonNull(settings, "settings"); Map sanitized = Utils.sanitizeNullFreeStringMap(settings); PlatformRecorder r = FlightRecorder.getFlightRecorder().getInternal(); synchronized (r) { - this.internal = r.newRecording(state, sanitized); + this.internal = r.newRecording(register, sanitized); this.internal.setRecording(this); if (internal.getRecording() != this) { throw new InternalError("Internal recording not properly setup"); @@ -503,7 +502,7 @@ public final class Recording implements Closeable { */ public void setName(String name) { Objects.requireNonNull(name, "name"); - internal.setName(name); + internal.setName(name, true); } /** diff --git a/src/jdk.jfr/share/classes/jdk/jfr/consumer/RecordingStream.java b/src/jdk.jfr/share/classes/jdk/jfr/consumer/RecordingStream.java index 5ec794bdf7c..22b08e04dc9 100644 --- a/src/jdk.jfr/share/classes/jdk/jfr/consumer/RecordingStream.java +++ b/src/jdk.jfr/share/classes/jdk/jfr/consumer/RecordingStream.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2019, 2025, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2019, 2026, 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 @@ -41,6 +41,7 @@ import jdk.jfr.EventSettings; import jdk.jfr.EventType; import jdk.jfr.Recording; import jdk.jfr.RecordingState; +import jdk.jfr.internal.PlatformRecorder; import jdk.jfr.internal.PlatformRecording; import jdk.jfr.internal.PrivateAccess; import jdk.jfr.internal.util.Utils; @@ -97,9 +98,9 @@ public final class RecordingStream implements AutoCloseable, EventStream { private RecordingStream(Map settings) { this.recording = new Recording(); this.creationTime = Instant.now(); - this.recording.setName("Recording Stream: " + creationTime); try { PlatformRecording pr = PrivateAccess.getInstance().getPlatformRecording(recording); + pr.setName("Recording Stream: " + creationTime, false); this.directoryStream = new EventDirectoryStream( null, pr, @@ -113,6 +114,12 @@ public final class RecordingStream implements AutoCloseable, EventStream { if (!settings.isEmpty()) { recording.setSettings(settings); } + PlatformRecorder recorder = PrivateAccess.getInstance().getPlatformRecorder(); + synchronized (recorder) { + if (recorder.isDestroyed()) { + close(); + } + } } private List configurations() { diff --git a/src/jdk.jfr/share/classes/jdk/jfr/internal/PlatformRecorder.java b/src/jdk.jfr/share/classes/jdk/jfr/internal/PlatformRecorder.java index 260f2fed54d..a82eff239e6 100644 --- a/src/jdk.jfr/share/classes/jdk/jfr/internal/PlatformRecorder.java +++ b/src/jdk.jfr/share/classes/jdk/jfr/internal/PlatformRecorder.java @@ -67,6 +67,7 @@ public final class PlatformRecorder { private long recordingCounter = 0; private RepositoryChunk currentChunk; private boolean runPeriodicTask; + private boolean destroyed; public PlatformRecorder() throws Exception { repository = Repository.getRepository(); @@ -82,8 +83,18 @@ public final class PlatformRecorder { Runtime.getRuntime().addShutdownHook(shutdownHook); } - public synchronized PlatformRecording newRecording(RecordingState state, Map settings) { - return newRecording(state, settings, ++recordingCounter); + public synchronized PlatformRecording newRecording(Boolean register, Map settings) { + long id = ++recordingCounter; + if (register == null) { + if (isDestroyed()) { + PlatformRecording r = newRecording(false, settings, id); + r.setState(RecordingState.CLOSED); + return r; + } else { + return newRecording(true, settings, id); + } + } + return newRecording(register, settings, id); } // To be used internally when doing dumps. @@ -92,15 +103,15 @@ public final class PlatformRecorder { if(!Thread.holdsLock(this)) { throw new InternalError("Caller must have recorder lock"); } - return newRecording(RecordingState.NEW, new HashMap<>(), 0); + return newRecording(true, new HashMap<>(), 0); } - private synchronized PlatformRecording newRecording(RecordingState state, Map settings, long id) { + private synchronized PlatformRecording newRecording(boolean register, Map settings, long id) { PlatformRecording recording = new PlatformRecording(this, id); if (!settings.isEmpty()) { recording.setSettings(settings); } - if (state != RecordingState.CLOSED) { + if (register) { recordings.add(recording); } return recording; @@ -177,8 +188,13 @@ public final class PlatformRecorder { } } } - writeReports(); + destroyed = true; + // This will prevent further interaction with recordings after JFR has been destroyed. + for (PlatformRecording p : getRecordings()) { + p.setState(RecordingState.CLOSED); + } + recordings.clear(); JDKEvents.remove(); if (JVMSupport.hasJFR()) { @@ -190,11 +206,29 @@ public final class PlatformRecorder { repository.clear(); } + // Not synchronized. Caller must hold recorder lock to avoid races. + public boolean isDestroyed() { + if (!Thread.holdsLock(this)) { + throw new InternalError("Caller must have recorder lock"); + } + return destroyed; + } + private void writeReports() { for (PlatformRecording recording : getRecordings()) { if (recording.isToDisk() && recording.getState() == RecordingState.STOPPED) { for (Report report : recording.getReports()) { - report.print(recording.getStartTime(), recording.getStopTime()); + try { + report.print(recording.getStartTime(), recording.getStopTime()); + } catch (Exception e) { + StringBuilder message = new StringBuilder(); + message.append("Could not generate report-on-exit for view "); + message.append(report.name()); + message.append(" (recording "); + message.append(recording.getName()).append(":").append(recording.getId()); + message.append("). Unexpected error: ").append(e.toString()); + Logger.log(JFR, WARN, message.toString()); + } } } } @@ -547,15 +581,15 @@ public final class PlatformRecorder { } synchronized Recording newCopy(PlatformRecording r, boolean stop) { - PrivateAccess pr = PrivateAccess.getInstance(); - boolean closed = r.getState() == RecordingState.CLOSED; - Recording newRec = closed ? pr.newRecording(RecordingState.CLOSED) : new Recording(); - PlatformRecording copy = pr.getPlatformRecording(newRec); + PrivateAccess access = PrivateAccess.getInstance(); + boolean register = !isDestroyed() && r.getState() != RecordingState.CLOSED; + Recording newRec = access.newRecording(register); + PlatformRecording copy = access.getPlatformRecording(newRec); copy.setSettings(r.getSettings()); copy.setMaxAge(r.getMaxAge()); copy.setMaxSize(r.getMaxSize()); copy.setDumpOnExit(r.getDumpOnExit()); - copy.setName("Clone of " + r.getName()); + copy.setName("Clone of " + r.getName(), false); copy.setToDisk(r.isToDisk()); copy.setInternalDuration(r.getDuration()); copy.setStartTime(r.getStartTime()); diff --git a/src/jdk.jfr/share/classes/jdk/jfr/internal/PlatformRecording.java b/src/jdk.jfr/share/classes/jdk/jfr/internal/PlatformRecording.java index 64454fc3cb4..92d60f5bdec 100644 --- a/src/jdk.jfr/share/classes/jdk/jfr/internal/PlatformRecording.java +++ b/src/jdk.jfr/share/classes/jdk/jfr/internal/PlatformRecording.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2016, 2025, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2016, 2026, 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 @@ -353,7 +353,7 @@ public final class PlatformRecording implements AutoCloseable { // Recording is RUNNING, create a clone PlatformRecording clone = recorder.newTemporaryRecording(); clone.setShouldWriteActiveRecordingEvent(false); - clone.setName(getName()); + clone.setName(getName(), false); clone.setToDisk(true); clone.setMaxAge(getMaxAge()); clone.setMaxSize(getMaxSize()); @@ -425,7 +425,7 @@ public final class PlatformRecording implements AutoCloseable { } } - void setState(RecordingState state) { + public void setState(RecordingState state) { synchronized (recorder) { this.state = state; } @@ -449,9 +449,11 @@ public final class PlatformRecording implements AutoCloseable { } } - public void setName(String name) { + public void setName(String name, boolean checkClosed) { synchronized (recorder) { - ensureNotClosed(); + if (checkClosed) { + ensureNotClosed(); + } this.name = name; } } diff --git a/src/jdk.jfr/share/classes/jdk/jfr/internal/PrivateAccess.java b/src/jdk.jfr/share/classes/jdk/jfr/internal/PrivateAccess.java index 2297bf7bdde..b30e50ff4f3 100644 --- a/src/jdk.jfr/share/classes/jdk/jfr/internal/PrivateAccess.java +++ b/src/jdk.jfr/share/classes/jdk/jfr/internal/PrivateAccess.java @@ -33,7 +33,6 @@ import jdk.jfr.Configuration; import jdk.jfr.EventSettings; import jdk.jfr.EventType; import jdk.jfr.Recording; -import jdk.jfr.RecordingState; import jdk.jfr.SettingDescriptor; import jdk.jfr.ValueDescriptor; import jdk.jfr.internal.management.EventSettingsModifier; @@ -97,7 +96,7 @@ public abstract class PrivateAccess { public abstract PlatformRecorder getPlatformRecorder(); - public abstract Recording newRecording(RecordingState state); + public abstract Recording newRecording(Boolean register); public abstract EventSettings newEventSettings(EventSettingsModifier esm); diff --git a/test/jdk/jdk/jfr/jvm/AfterShutdown.java b/test/jdk/jdk/jfr/jvm/AfterShutdown.java new file mode 100644 index 00000000000..3c822a47cd8 --- /dev/null +++ b/test/jdk/jdk/jfr/jvm/AfterShutdown.java @@ -0,0 +1,190 @@ +/* + * Copyright (c) 2026, 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.jvm; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; + +import jdk.jfr.Recording; +import jdk.jfr.FlightRecorder; +import jdk.jfr.RecordingState; +import jdk.jfr.consumer.RecordingStream; + +/** + * Test application that tries to interact with JFR after shutdown. + */ +@SuppressWarnings("resource") +public class AfterShutdown { + private static boolean fail; + + public static void main(String... args) throws Exception { + Recording fresh = new Recording(); + + Recording started = new Recording(); + started.start(); + + Recording stopped = new Recording(); + stopped.start(); + stopped.stop(); + + Recording closed = new Recording(); + closed.close(); + + Recording fullCycle = new Recording(); + fullCycle.start(); + fullCycle.stop(); + fullCycle.close(); + + Recording copyable = new Recording(); + copyable.start(); + + Runtime.getRuntime().addShutdownHook(new Thread() { + public void run() { + try { + awaitRepositoryRemoved(); + assertClosedBehavior(fresh, "new recording"); + assertClosedBehavior(started, "started recording"); + assertClosedBehavior(stopped, "stopped recording"); + assertClosedBehavior(closed, "closed recording"); + assertClosedBehavior(fullCycle, "full-cycle recording"); + testCopy(copyable); + testSnapshot(); + testLateRecording(); + testLateRecordingStream(); + } catch (Throwable t) { + fail = true; + t.printStackTrace(); + } + if (!fail) { + System.out.println("PASS"); + } + } + }); + } + + private static void testLateRecording() { + Recording r = new Recording(); + assertClosedBehavior(r, "late recording"); + } + + private static void testCopy(Recording r) { + try (Recording copy = r.copy(false)) { + assertClosedBehavior(copy, "copied recording with stopped=true"); + } + try (Recording copy = r.copy(true)) { + assertClosedBehavior(copy, "copied recording with stopped=false"); + } + } + + private static void testSnapshot() { + try (Recording snapshot = FlightRecorder.getFlightRecorder().takeSnapshot()) { + assertClosedBehavior(snapshot, "snapshot"); + } + } + + public static void assertClosedBehavior(Recording recording, String kind) { + assertClosed(recording); + try { + recording.start(); + fail("Expected IllegalStateException if " + kind + " is started after shutdown."); + } catch (IllegalStateException ise) { + // As expected + } + try { + recording.dump(Path.of("file.jfr")); + fail("Expected IOException if " + kind + " is dumped after shutdown"); + } catch (IOException ioe) { + // As expected + } + try { + recording.stop(); + fail("Expected IllegalStateException if " + kind + " is stopped after shutdown"); + } catch (IllegalStateException ioe) { + // As expected + } + try { + recording.close(); + } catch (Exception e) { + e.printStackTrace(); + fail("Should be able to close " + kind + " after shutdown."); + } + } + + private static void testLateRecordingStream() { + RecordingStream rs = new RecordingStream(); + assertClosedBehavior(rs, "late recording stream"); + } + + public static void assertClosedBehavior(RecordingStream rs, String kind) { + try { + rs.start(); + fail("Expected IllegalStateException if " + kind + " is started after shutdown."); + } catch (IllegalStateException ise) { + // As expected + } + try { + rs.dump(Path.of("file.jfr")); + fail("Expected IOException if " + kind + " is dumped after shutdown"); + } catch (IOException ioe) { + // As expected + } + try { + rs.stop(); + fail("Expected IllegalStateException if " + kind + " is stopped after shutdown"); + } catch (IllegalStateException ioe) { + // As expected + } + try { + rs.close(); + } catch (Exception e) { + e.printStackTrace(); + fail("Should be able to close " + kind + " after shutdown."); + } + } + + private static void awaitRepositoryRemoved() { + String path = System.getProperty("jdk.jfr.repository"); + Path repository = Path.of(path); + while (Files.exists(repository)) { + System.out.println("Repository still exist. Waiting 100 ms ..."); + try { + Thread.sleep(100); + } catch (InterruptedException ie) { + Thread.currentThread().interrupt(); + } + } + System.out.println("Repository removed."); + } + + private static void assertClosed(Recording r) { + if (r.getState() != RecordingState.CLOSED) { + fail("Recording was in state " + r.getState() + " but expected it to be CLOSED"); + } + } + + private static void fail(String message) { + System.out.println("FAIL: " + message); + fail = true; + } +} diff --git a/test/jdk/jdk/jfr/jvm/TestAfterShutdown.java b/test/jdk/jdk/jfr/jvm/TestAfterShutdown.java new file mode 100644 index 00000000000..7920a1397b5 --- /dev/null +++ b/test/jdk/jdk/jfr/jvm/TestAfterShutdown.java @@ -0,0 +1,44 @@ +/* + * Copyright (c) 2026, 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.jvm; + +import jdk.test.lib.process.ProcessTools; +/** + * @test TestAfterShutdown + * @requires vm.flagless + * @summary Checks that API interactions with JFR after shutdown works as expected + * @requires vm.hasJFR + * @library /test/lib + * @build jdk.jfr.jvm.AfterShutdown + * @run main/othervm jdk.jfr.jvm.TestAfterShutdown + */ +public class TestAfterShutdown { + public static void main(String... args) throws Exception { + var pb = ProcessTools.createTestJavaProcessBuilder(AfterShutdown.class.getName()); + var result = ProcessTools.executeProcess(pb); + result.shouldHaveExitValue(0); + result.shouldNotContain("FAIL"); + result.shouldContain("PASS"); + } +} \ No newline at end of file