8390670: [CRaC] Fix restart of streaming JFR recordings by rvansa · Pull Request #345 · openjdk/crac

rvansa · 2026-08-19T14:09:10Z

When a recording does not have its destination set we hit a NPE.

There's more problems beyond the NPE, though: the EventDirectoryStream used by RecordingStream can keep a file open.

Also, currently all recordings are attempted to be restarted (recreated). We should do that only with the running ones; not started and closed recordings can be left as-is.

I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

Progress

Change must not contain extraneous whitespace

Issue

JDK-8390670: [CRaC] Fix restart of streaming JFR recordings (Bug - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/crac.git pull/345/head:pull/345
$ git checkout pull/345

Update a local copy of the PR:
$ git checkout pull/345
$ git pull https://git.openjdk.org/crac.git pull/345/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 345

View PR using the GUI difftool:
$ git pr show -t 345

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/crac/pull/345.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2026-08-19T14:11:18Z

👋 Welcome back rvansa! A progress list of the required criteria for merging this PR into crac will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2026-08-19T14:11:37Z

@rvansa This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8390670: [CRaC] Fix restart of streaming JFR recordings

You can use pull request commands such as /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1 new commit pushed to the crac branch:

d31e8dc: 8390453: [CRaC] Custom image constraints

Please see this link for an up-to-date comparison between the source branch of this pull request and the crac branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the crac branch, type /integrate in a new comment.

mlbridge · 2026-08-19T14:16:06Z

Webrevs

TimPushkin · 2026-08-24T14:14:02Z

+ *
+ * @crac The recording is automatically stopped on checkpoint and re-created on restore.


There's no official recommendation but looks like we've been putting this as the very first tag.

Same comment for Recording

TimPushkin · 2026-08-25T06:51:52Z

+                    PlatformRecording pr = access.getPlatformRecording(rec);
+                    // Update the owning Recording (important if this was created programmatically)
+                    access.setPlatformRecording(r.getRecording(), pr);


Here we create a new Recording (let's call it R1) with the same settings as some old one (let's call it R0), it creates a new PlatformRecording, then we make the old R0 point to the new PlatformRecording. But we never make the new PlatformRecording point to R0, it remains pointing to R1. So, from the second checkpoint onwards we don't have a link to R0, the actual Recording that the user code holds, and it stops getting updated.

Also it's hard to understand the difference between r, pr, rec. I think even something like oldRecording, oldPlatformRecording, newRecording, newPlatformRecording (maybe shortened, but keeping the old/new platform/non-platform pairs visible) would make it more readable.

TimPushkin · 2026-08-25T06:57:07Z

+        private void backupOriginalDestination(WriteablePath original) {
+            try {
+                File destFile = original.getReal().toFile();
+                if (destFile.exists()) {


Maybe early return to reduce the nesting?

TimPushkin · 2026-08-25T06:59:30Z

+/**
+ * @test FlightRecorderStreamingTest
+ * @library /test/lib
+ * @requires (os.family == "linux")


Why do we need Linux for this, wouldn't simengine suffice?

Same question for the other test

TimPushkin · 2026-08-25T07:14:46Z

                        Logger.log(LogTag.JFR_SYSTEM_PARSER, LogLevel.INFO, "Unexpected chunk with 0 ns duration");
                    }
                }
                path = repositoryFiles.nextPath(currentChunkStartNanos + durationNanos, getWaitForChunks());


This thing opens directories and files in a loop outside the lock. AI found this, worth checking the rest of the code outside the lock.

TimPushkin · 2026-08-25T07:46:26Z

    private boolean destroyed;
    private JDKResource resource = new JDKResource() {
        private List<PlatformRecording> futureRecordings;
        private static int MAX_BACKUPS = Integer.getInteger("jdk.jfr.max_backups", 20);


Should be final. Or maybe we could update it on restore?

TimPushkin · 2026-08-25T08:34:43Z

+                lock.lock();
                input.setFile(path);


IIUC the PlatformRecorder's resource may change the path and delete the old file (by closing the recording) while this thread waits on the lock, then this setFile will fail since the file on the old path does not exist anymore

TimPushkin · 2026-08-25T08:36:13Z

                    rec.setMaxAge(r.getMaxAge());
                    rec.setMaxSize(r.getMaxSize());
                    pr.setInternalDuration(r.getDuration());
                    rec.setDumpOnExit(r.getDumpOnExit());
                    pr.setFlushInterval(r.getFlushInterval());


reports should also be copied

TimPushkin · 2026-08-25T08:43:28Z

+        if (file != null) {
+            try {
+                file.close();
+            } catch (IOException e) {
+                // perhaps deleted
+            }
+            file = null;
        }


Why is the null check needed? The only place where file is set to null is in this method which then sets in back to non-null unconditionally. The only way it remains null is if this methods throws, but the only caller (the EventDirectoryStream loop) propagates the exception out of the scope where this object exists, so another setFile call is not possible.

The null check here is good as a defense, but since it does not seem CRaC-related for us it's a needless difference from the mainline.

TimPushkin · 2026-08-25T08:59:08Z

+    @Override
+    public void beforeCheckpoint(Context<? extends Resource> context) throws Exception {
+        // Ensures that the thread in processRecursionSafe() does not have an open file
+        lock.lock();


I wonder if we should put some timeout here. AI tells me this wait can last forever at least in these cases (the reproducers are also AI-generated, I didn't test them):

The stream has an onEvent handler that got blocked — probably a user's fault for which it's OK to stall.

var queue = new ArrayBlockingQueue<RecordedEvent>(10); // nobody draining it try (var rs = new RecordingStream()) { rs.enable("jdk.CPULoad").withPeriod(Duration.ofMillis(1)); rs.onEvent("jdk.CPULoad", e -> { try { queue.put(e); } catch (InterruptedException ignored) {} // blocks once full }); rs.startAsync(); Thread.sleep(1000); // let the queue fill CRaCMXBean.getCRaCMXBean().checkpointRestore(); // never returns }

Streams with recording == null where the writer never finishes, e.g. created via EventStream.openRepository to a repository of another JVM that died — this seems like something we should not stall on.

$ java -XX:StartFlightRecording -XX:FlightRecorderOptions:repository=/tmp/repo Foo & $ sleep 2 && kill -9 %1 # chunk left with fileState != 0, never finalized // in the JVM being checkpointed var es = EventStream.openRepository(Path.of("/tmp/repo/<subdir>")); es.onEvent(e -> {}); es.startAsync(); CRaCMXBean.getCRaCMXBean().checkpointRestore(); // never returns

8390670: [CRaC] Fix restart of streaming JFR recordings

89d8c33

openjdk Bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 19, 2026

rvansa added 2 commits August 20, 2026 09:55

Drop accidentally committed newline

290eae8

Fix javadoc

d6fc379

rvansa requested a review from TimPushkin August 20, 2026 13:36

TimPushkin reviewed Aug 25, 2026

View reviewed changes

		*
		* @crac The recording is automatically stopped on checkpoint and re-created on restore.

Uh oh!

Conversation

rvansa commented Aug 19, 2026 • edited by openjdk Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewing

Uh oh!

bridgekeeper Bot commented Aug 19, 2026

Uh oh!

openjdk Bot commented Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge Bot commented Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

rvansa commented Aug 19, 2026 •

edited by openjdk Bot

Loading

openjdk Bot commented Aug 19, 2026 •

edited

Loading

mlbridge Bot commented Aug 19, 2026 •

edited

Loading