8365066: RecordingStream and RemoteRecordingStream do not terminate when the associated Recording is stopped or closed externally #26710

chiroito · 2025-08-09T01:33:29Z

Fix an issue where RecordingStream and RemoteRecordingStream do not stop when their underlying the local/remote recording stop.

Introduce an internal EventSource abstraction to unify control across local (PlatformRecording) and remote (FlightRecorderMXBean) recordings, ensuring consistent propagation of stop/close and stop time to the consumer streams.

Tests updated to validate lifecycle consistency:
Verifies that stopping/closing the underlying recording (local and remote) properly stops/closes the corresponding stream.

Test: jdk/jdk/jfr

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8365066: RecordingStream and RemoteRecordingStream do not terminate when the associated Recording is stopped or closed externally (Bug - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26710

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26710.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-08-09T01:34:03Z

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

openjdk · 2025-08-09T01:35:48Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2025-08-09T01:36:40Z

@chiroito The following label will be automatically applied to this pull request:

hotspot-jfr

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

egahlin · 2025-08-09T04:50:26Z

Did you look into adding a FlightRecorderListener and implementing recordingStateChanged to check if the RecordingState is CLOSED and then call close() on the stream? Or perhaps it would be better to set a flag and let the thread running the stream close itself after the next flush is complete.

For a RemoteRecordingStream there is a JMX notification that can be listened on.

turbanoff · 2025-08-12T06:53:55Z

test/jdk/jdk/jfr/api/consumer/recordingstream/TestClosedRecording.java

+    private static class SendEventListener implements FlightRecorderListener {
+        @Override
+        public void recordingStateChanged(Recording recording) {
+            if(recording.getState() == RecordingState.RUNNING){


Suggested change

if(recording.getState() == RecordingState.RUNNING){

if (recording.getState() == RecordingState.RUNNING){

turbanoff · 2025-08-12T06:54:05Z

test/jdk/jdk/jfr/jmx/streaming/TestClosedRecording.java

+    private static class SendEventListener implements FlightRecorderListener {
+        @Override
+        public void recordingStateChanged(Recording recording) {
+            if(recording.getState() == RecordingState.RUNNING){


Suggested change

if(recording.getState() == RecordingState.RUNNING){

if (recording.getState() == RecordingState.RUNNING){

turbanoff · 2025-08-12T06:54:20Z

src/jdk.management.jfr/share/classes/jdk/management/jfr/RemoteRecordingStream.java

@@ -469,7 +467,9 @@ public void close() {
        ManagementSupport.setOnChunkCompleteHandler(stream, null);
        stream.close();
        try {
-            mbean.closeRecording(recordingId);
+            if(eventSource.getState() != RecordingState.CLOSED) {


Suggested change

if(eventSource.getState() != RecordingState.CLOSED) {

if (eventSource.getState() != RecordingState.CLOSED) {

turbanoff · 2025-08-12T06:54:38Z

src/jdk.jfr/share/classes/jdk/jfr/internal/consumer/LocalRecordingEventSource.java

+    }
+
+    @Override
+    public RecordingState getState()  {


Suggested change

public RecordingState getState() {

public RecordingState getState() {

turbanoff · 2025-08-12T06:54:56Z

src/jdk.jfr/share/classes/jdk/jfr/internal/consumer/EventDirectoryStream.java

@@ -169,6 +167,19 @@ protected void processRecursionSafe() throws IOException {
                                     "ns (epoch), parser at " + lastFlush + "ns (epoch).");
                        return;
                    }
+
+                    if(!barrier.used()) {


Suggested change

if(!barrier.used()) {

if (!barrier.used()) {

turbanoff · 2025-08-12T06:55:04Z

src/jdk.jfr/share/classes/jdk/jfr/internal/consumer/EventDirectoryStream.java

-                if (isRecordingStream()) {
-                    if (recording.getState() == RecordingState.STOPPED && !barrier.used()) {
-                        logStreamEnd("recording stopped externally.");
+                if(!barrier.used()) {


Suggested change

if(!barrier.used()) {

if (!barrier.used()) {

turbanoff · 2025-08-12T08:08:30Z

src/jdk.jfr/share/classes/jdk/jfr/internal/consumer/EventDirectoryStream.java

+                    if (state == RecordingState.CLOSED){
+                        logStreamEnd("Event source is closed externally");
+                        return;
+                    }else if(state == RecordingState.STOPPED) {


Suggested change

}else if(state == RecordingState.STOPPED) {

} else if (state == RecordingState.STOPPED) {

turbanoff · 2025-08-12T08:08:43Z

test/jdk/jdk/jfr/api/consumer/recordingstream/TestStoppedRecording.java

+    private static class SendEventListener implements  FlightRecorderListener{
+        @Override
+        public void recordingStateChanged(Recording recording) {
+            if(recording.getState() == RecordingState.RUNNING){


Suggested change

if(recording.getState() == RecordingState.RUNNING){

if (recording.getState() == RecordingState.RUNNING){

turbanoff · 2025-08-12T08:08:56Z

test/jdk/jdk/jfr/jmx/streaming/TestStoppedRecording.java

+    private static class SendEventListener implements FlightRecorderListener {
+        @Override
+        public void recordingStateChanged(Recording recording) {
+            if(recording.getState() == RecordingState.RUNNING){


Suggested change

if(recording.getState() == RecordingState.RUNNING){

if (recording.getState() == RecordingState.RUNNING){

turbanoff · 2025-08-12T08:09:20Z

test/jdk/jdk/jfr/api/consumer/recordingstream/TestStoppedRecording.java

@@ -40,21 +41,44 @@
 */
 public class TestStoppedRecording {

+    private static class SendEventListener implements  FlightRecorderListener{


Suggested change

private static class SendEventListener implements FlightRecorderListener{

private static class SendEventListener implements FlightRecorderListener {

turbanoff · 2025-08-12T08:09:41Z

...jdk.management.jfr/share/classes/jdk/management/jfr/internal/RemoteRecordingEventSource.java

+    @Override
+    public long getStopTime() {
+        Optional<RecordingInfo> recordingInfo = mbean.getRecordings().stream().filter(r -> r.getId() == recordingId).findFirst();
+        if(recordingInfo.isEmpty()){


Suggested change

if(recordingInfo.isEmpty()){

if (recordingInfo.isEmpty()){

turbanoff · 2025-08-12T08:09:56Z

...jdk.management.jfr/share/classes/jdk/management/jfr/internal/RemoteRecordingEventSource.java

+    }
+
+    @Override
+    public RecordingState getState()  {


Suggested change

public RecordingState getState() {

public RecordingState getState() {

turbanoff · 2025-08-12T08:10:03Z

...jdk.management.jfr/share/classes/jdk/management/jfr/internal/RemoteRecordingEventSource.java

+    @Override
+    public RecordingState getState()  {
+        Optional<RecordingInfo> recordingInfo = mbean.getRecordings().stream().filter(r -> r.getId() == recordingId).findFirst();
+        if(recordingInfo.isEmpty()){


Suggested change

if(recordingInfo.isEmpty()){

if (recordingInfo.isEmpty()){

turbanoff · 2025-08-12T08:10:10Z

...jdk.management.jfr/share/classes/jdk/management/jfr/internal/RemoteRecordingEventSource.java

+    public RecordingState getState()  {
+        Optional<RecordingInfo> recordingInfo = mbean.getRecordings().stream().filter(r -> r.getId() == recordingId).findFirst();
+        if(recordingInfo.isEmpty()){
+            return  RecordingState.CLOSED;


Suggested change

return RecordingState.CLOSED;

return RecordingState.CLOSED;

chiroito · 2025-08-12T12:21:39Z

@egahlin That sounds like a good idea. I’ll give it a try.

…hen the associated Recording is stopped or closed externally

openjdk · 2025-08-21T09:23:57Z

@chiroito Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

mlbridge · 2025-08-21T09:27:42Z

Webrevs

00: Full (5b437d7a)

chiroito · 2025-08-21T14:35:02Z

@egahlin
Could you please review this please?
I added a listener-based mechanism that propagates external stop/close to the stream.

egahlin · 2025-08-21T18:12:46Z

@egahlin Could you please review this please? I added a listener-based mechanism that propagates external stop/close to the stream.

This will take me some time to go through. I'm a little bit worried about the locking, but I need to investigate, before I can comment further.

chiroito · 2025-08-24T04:11:23Z

@egahlin

I apologize.
I forgot to fix some code I had been using to test the locking behavior and mistakenly committed it.
I’ll correct this and push the changes soon.

I do have one question: is there a general preference between using synchronized and ReentrantLock?
Personally, I lean toward ReentrantLock, since it can be safely used with virtual threads and may be easier to backport if needed.

egahlin · 2025-08-24T07:46:24Z

@egahlin

I apologize. I forgot to fix some code I had been using to test the locking behavior and mistakenly committed it. I’ll correct this and push the changes soon.

I do have one question: is there a general preference between using synchronized and ReentrantLock? Personally, I lean toward ReentrantLock, since it can be safely used with virtual threads and may be easier to backport if needed.

The preference is to not use locks at all when callbacks (listeners) are being invoked as it could potentially lead to dead locks. Best would be if a flag can be set so the stream stops or closes itself at the next flush, but again, I haven't had time to look at it, so it might not be feasible. Best behavior would be if an external stop or close acted as an internal one. The code with the stream barrier is complex, but it was needed to ensure all subscribed events emitted before a stop gets an onEvent(...) callback.

egahlin · 2025-08-31T23:03:34Z

I've looked into the issue, and I'm not sure a RemoteRecordingStream should be closed if the underlying recording is.

One use case for the RemoteRecordingStream is to replicate the repository on another host. When the monitored application exits (and closes the recording), the data should be available on another machine. If we close the stream, data will be lost (prematurely). I need to think about this some more.
I don't think we need to do anything when the underlying recording is stopped for RemoteRecordingStream, since a stopped stream should wait until all events in the recording have been consumed. The DownloadThread will end when there is no more data to be read from FlightRecorderMXBean::readStream.
For a RecordingStream, I think Recording::stop/close can just delegate to RecordingStream::stop/close and we can get the exact same behavior as if the stream was stopped/closed.

I have implemented the above changes. You can look at it here #27025

I noticed that the underlying Recording object may be exposed before the stream has been initialized, so I added synchronization. Now, any other thread trying to get a recording will have to wait for the RecordingStream initialization to complete.

openjdk bot added the hotspot-jfr [email protected] label Aug 9, 2025

turbanoff reviewed Aug 12, 2025

View reviewed changes

8365066: RecordingStream and RemoteRecordingStream do not terminate w…

5b437d7

…hen the associated Recording is stopped or closed externally

chiroito force-pushed the recording-stream branch from 27b52e6 to 5b437d7 Compare August 21, 2025 09:23

openjdk bot added the rfr Pull request is ready for review label Aug 21, 2025

	if(recording.getState() == RecordingState.RUNNING){
	if (recording.getState() == RecordingState.RUNNING){

	if(eventSource.getState() != RecordingState.CLOSED) {
	if (eventSource.getState() != RecordingState.CLOSED) {

	public RecordingState getState() {
	public RecordingState getState() {

	}else if(state == RecordingState.STOPPED) {
	} else if (state == RecordingState.STOPPED) {

	private static class SendEventListener implements FlightRecorderListener{
	private static class SendEventListener implements FlightRecorderListener {

8365066: RecordingStream and RemoteRecordingStream do not terminate when the associated Recording is stopped or closed externally #26710

Are you sure you want to change the base?

8365066: RecordingStream and RemoteRecordingStream do not terminate when the associated Recording is stopped or closed externally #26710

Conversation

chiroito commented Aug 9, 2025 • 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 9, 2025

Uh oh!

openjdk bot commented Aug 9, 2025

Uh oh!

openjdk bot commented Aug 9, 2025

Uh oh!

egahlin commented Aug 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

turbanoff Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

chiroito commented Aug 12, 2025

Uh oh!

openjdk bot commented Aug 21, 2025

Uh oh!

mlbridge bot commented Aug 21, 2025

Webrevs

Uh oh!

chiroito commented Aug 21, 2025

Uh oh!

egahlin commented Aug 21, 2025

Uh oh!

chiroito commented Aug 24, 2025

Uh oh!

egahlin commented Aug 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

egahlin commented Aug 31, 2025

Uh oh!

Uh oh!

chiroito commented Aug 9, 2025 •

edited by openjdk bot

Loading

egahlin commented Aug 9, 2025 •

edited

Loading

turbanoff Aug 12, 2025 •

edited

Loading

egahlin commented Aug 24, 2025 •

edited

Loading