RFR: 8364427: JFR: Possible resource leak in Recording::getStream
Could I have a review of the PR that fixes resource leaks when Recording::getStream is used? Testing: tier1 + test/jdk/jdk/jfr Thanks Erik ------------- Commit messages: - Fix whitespace - Initial Changes: https://git.openjdk.org/jdk/pull/26575/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=26575&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8364427 Stats: 176 lines in 2 files changed: 160 ins; 6 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/26575.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/26575/head:pull/26575 PR: https://git.openjdk.org/jdk/pull/26575
On Thu, 31 Jul 2025 12:17:47 GMT, Erik Gahlin <egahlin@openjdk.org> wrote:
Could I have a review of the PR that fixes resource leaks when Recording::getStream is used?
Testing: tier1 + test/jdk/jdk/jfr
Thanks Erik
src/jdk.jfr/share/classes/jdk/jfr/internal/ChunkInputStream.java line 69:
67: while (nextChunk()) { 68: try { 69: stream = new BufferedInputStream(Files.newInputStream(currentChunk.getFile()));
Should we close `stream` if it's not null? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/26575#discussion_r2245321075
On Thu, 31 Jul 2025 12:59:10 GMT, Francesco Andreuzzi <duke@openjdk.org> wrote:
Could I have a review of the PR that fixes resource leaks when Recording::getStream is used?
Testing: tier1 + test/jdk/jdk/jfr
Thanks Erik
src/jdk.jfr/share/classes/jdk/jfr/internal/ChunkInputStream.java line 69:
67: while (nextChunk()) { 68: try { 69: stream = new BufferedInputStream(Files.newInputStream(currentChunk.getFile()));
Should we close `stream` if it's not null?
Could you elaborate? There is closeStream method that is called when there is no more data to be read. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/26575#discussion_r2245341547
On Thu, 31 Jul 2025 13:05:58 GMT, Erik Gahlin <egahlin@openjdk.org> wrote:
src/jdk.jfr/share/classes/jdk/jfr/internal/ChunkInputStream.java line 69:
67: while (nextChunk()) { 68: try { 69: stream = new BufferedInputStream(Files.newInputStream(currentChunk.getFile()));
Should we close `stream` if it's not null?
Could you elaborate?
There is closeStream method that is called when there is no more data to be read.
If `nextChunk` returns `true` e.g. 2 times in a row, the second iteration of the while loop at L67 will overwrite `stream` without closing the previous instance. I don't see if/where `closeStream` will be called within two iterations of the loop, maybe I'm missing something? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/26575#discussion_r2245367507
On Thu, 31 Jul 2025 13:13:53 GMT, Francesco Andreuzzi <duke@openjdk.org> wrote:
Could you elaborate?
There is the closeStream method that is called when there is no more data to be read.
If `nextChunk` returns `true` e.g. 2 times in a row, the second iteration of the while loop at L67 will overwrite `stream` without closing the previous instance. I don't see if/where `closeStream` will be called within two iterations of the loop, maybe I'm missing something?
I think it's closed by the read methods or close(). Just to be sure, I inserted: + if (this.stream != null) { + throw new InternalError("Should not happen"); + } stream = new BufferedInputStream(Files.newInputStream(currentChunk.getFile())); and ran all the tests without failure. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/26575#discussion_r2245424612
On Thu, 31 Jul 2025 13:35:35 GMT, Erik Gahlin <egahlin@openjdk.org> wrote:
If `nextChunk` returns `true` e.g. 2 times in a row, the second iteration of the while loop at L67 will overwrite `stream` without closing the previous instance. I don't see if/where `closeStream` will be called within two iterations of the loop, maybe I'm missing something?
I think it's closed by the read methods or close(). Just to be sure, I inserted:
+ if (this.stream != null) { + throw new InternalError("Should not happen"); + } stream = new BufferedInputStream(Files.newInputStream(currentChunk.getFile()));
and ran all the tests without failure.
Thanks for the clarification ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/26575#discussion_r2245429005
On Thu, 31 Jul 2025 12:17:47 GMT, Erik Gahlin <egahlin@openjdk.org> wrote:
Could I have a review of the PR that fixes resource leaks when Recording::getStream is used?
Testing: tier1 + test/jdk/jdk/jfr
Thanks Erik
Marked as reviewed by mgronlun (Reviewer). ------------- PR Review: https://git.openjdk.org/jdk/pull/26575#pullrequestreview-3080180909
On Thu, 31 Jul 2025 12:17:47 GMT, Erik Gahlin <egahlin@openjdk.org> wrote:
Could I have a review of the PR that fixes resource leaks when Recording::getStream is used?
Testing: tier1 + test/jdk/jdk/jfr
Thanks Erik
This pull request has now been integrated. Changeset: cf5a2553 Author: Erik Gahlin <egahlin@openjdk.org> URL: https://git.openjdk.org/jdk/commit/cf5a25538e09e449ff621562df6529abaa9b3685 Stats: 176 lines in 2 files changed: 160 ins; 6 del; 10 mod 8364427: JFR: Possible resource leak in Recording::getStream Reviewed-by: mgronlun ------------- PR: https://git.openjdk.org/jdk/pull/26575
participants (3)
-
Erik Gahlin
-
Francesco Andreuzzi
-
Markus Grönlund