RFR: JDK-8265078: jpackage tests on Windows leave large temp files
two changes: One to jpackage, when recursively removing directory, when IOException occurs, record it and continue (deleting as much as possible) before throwing the exception. The other to tests, when running jpackage via ProcessBuilder.execute(), set the "TMP" environment variable to the current value of System Property "java.io.tmpdir". This causes the sub-process (jpackage) to output tmp files to the tmp file location used by the test. (So the test harness can clean up after test exits). ------------- Commit messages: - JDK-8265078: jpackage tests on Windows leave large temp files Changes: https://git.openjdk.java.net/jdk/pull/3473/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3473&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8265078 Stats: 29 lines in 3 files changed: 27 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/3473.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3473/head:pull/3473 PR: https://git.openjdk.java.net/jdk/pull/3473
On Tue, 13 Apr 2021 18:57:20 GMT, Andy Herrick <herrick@openjdk.org> wrote:
two changes: One to jpackage, when recursively removing directory, when IOException occurs, record it and continue (deleting as much as possible) before throwing the exception. The other to tests, when running jpackage via ProcessBuilder.execute(), set the "TMP" environment variable to the current value of System Property "java.io.tmpdir". This causes the sub-process (jpackage) to output tmp files to the tmp file location used by the test. (So the test harness can clean up after test exits).
Changes requested by asemenyuk (Reviewer). src/jdk.jpackage/share/classes/jdk/jpackage/internal/IOUtils.java line 59:
57: 58: public static void deleteRecursive(Path directory) throws IOException { 59: final IOException [] exception = { (IOException) null };
Do we know `Files.walkFileTree()` synchronizes calls on callback object? If not, I'd use `AtomicReference` to store the first exception. test/jdk/tools/jpackage/helpers/jdk/jpackage/test/Executor.java line 92:
90: } 91: 92: public Executor setTmpDir(String tmp) {
As this would work only on Windows, I'd throw `IllegalStateException` if the method is called on other platform. test/jdk/tools/jpackage/helpers/jdk/jpackage/test/JPackageCommand.java line 646:
644: } else { 645: exec.setExecutable(JavaTool.JPACKAGE); 646: exec.setTmpDir(System.getProperty("java.io.tmpdir"));
This would work only on Windows. I'd put corresponding `if` around this statement to avoid future confusion. ------------- PR: https://git.openjdk.java.net/jdk/pull/3473
On Tue, 13 Apr 2021 19:48:24 GMT, Alexey Semenyuk <asemenyuk@openjdk.org> wrote:
two changes: One to jpackage, when recursively removing directory, when IOException occurs, record it and continue (deleting as much as possible) before throwing the exception. The other to tests, when running jpackage via ProcessBuilder.execute(), set the "TMP" environment variable to the current value of System Property "java.io.tmpdir". This causes the sub-process (jpackage) to output tmp files to the tmp file location used by the test. (So the test harness can clean up after test exits).
src/jdk.jpackage/share/classes/jdk/jpackage/internal/IOUtils.java line 59:
57: 58: public static void deleteRecursive(Path directory) throws IOException { 59: final IOException [] exception = { (IOException) null };
Do we know `Files.walkFileTree()` synchronizes calls on callback object? If not, I'd use `AtomicReference` to store the first exception.
That seems like overkill. walkFileTree must call visitFile, preVisitDirectory, and postVisitDirectory synchronously, because their return value tells walkFileTree where to go next.
test/jdk/tools/jpackage/helpers/jdk/jpackage/test/JPackageCommand.java line 646:
644: } else { 645: exec.setExecutable(JavaTool.JPACKAGE); 646: exec.setTmpDir(System.getProperty("java.io.tmpdir"));
This would work only on Windows. I'd put corresponding `if` around this statement to avoid future confusion.
That makes sense, should this be here in JPackageCommand.CreateExecutor() or in Executor.setTempDir() (maybe renamed setWinTempDir ?) ------------- PR: https://git.openjdk.java.net/jdk/pull/3473
On Tue, 13 Apr 2021 20:26:56 GMT, Andy Herrick <herrick@openjdk.org> wrote:
src/jdk.jpackage/share/classes/jdk/jpackage/internal/IOUtils.java line 59:
57: 58: public static void deleteRecursive(Path directory) throws IOException { 59: final IOException [] exception = { (IOException) null };
Do we know `Files.walkFileTree()` synchronizes calls on callback object? If not, I'd use `AtomicReference` to store the first exception.
That seems like overkill. walkFileTree must call visitFile, preVisitDirectory, and postVisitDirectory synchronously, because their return value tells walkFileTree where to go next.
I can use AtomicReference instead of Array to hold the IOException, but must I lock around access, there is no "setIfNull()" method ------------- PR: https://git.openjdk.java.net/jdk/pull/3473
On Tue, 13 Apr 2021 20:31:38 GMT, Andy Herrick <herrick@openjdk.org> wrote:
That seems like overkill. walkFileTree must call visitFile, preVisitDirectory, and postVisitDirectory synchronously, because their return value tells walkFileTree where to go next.
I can use AtomicReference instead of Array to hold the IOException, but must I lock around access, there is no "setIfNull()" method
Actually there is: compareAndSet(newValue, null) ------------- PR: https://git.openjdk.java.net/jdk/pull/3473
two changes: One to jpackage, when recursively removing directory, when IOException occurs, record it and continue (deleting as much as possible) before throwing the exception. The other to tests, when running jpackage via ProcessBuilder.execute(), set the "TMP" environment variable to the current value of System Property "java.io.tmpdir". This causes the sub-process (jpackage) to output tmp files to the tmp file location used by the test. (So the test harness can clean up after test exits).
Andy Herrick has updated the pull request incrementally with one additional commit since the last revision: JDK-8265078: jpackage tests on Windows leave large temp files ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/3473/files - new: https://git.openjdk.java.net/jdk/pull/3473/files/69949578..e78442ba Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3473&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3473&range=00-01 Stats: 19 lines in 3 files changed: 3 ins; 4 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/3473.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3473/head:pull/3473 PR: https://git.openjdk.java.net/jdk/pull/3473
On Tue, 13 Apr 2021 21:05:26 GMT, Andy Herrick <herrick@openjdk.org> wrote:
two changes: One to jpackage, when recursively removing directory, when IOException occurs, record it and continue (deleting as much as possible) before throwing the exception. The other to tests, when running jpackage via ProcessBuilder.execute(), set the "TMP" environment variable to the current value of System Property "java.io.tmpdir". This causes the sub-process (jpackage) to output tmp files to the tmp file location used by the test. (So the test harness can clean up after test exits).
Andy Herrick has updated the pull request incrementally with one additional commit since the last revision:
JDK-8265078: jpackage tests on Windows leave large temp files
Are you sure you need an `AtomicReference` to set the exception? I don't see any use of multiple threads, but I might be missing something. If you do need it, you need to fix the order of arguments. src/jdk.jpackage/share/classes/jdk/jpackage/internal/IOUtils.java line 96:
94: Files.delete(dir); 95: } catch (IOException ex) { 96: exception.compareAndSet(ex, null);
The arguments are backwards. The first argument is the one used for comparison, and if the current reference is equal to the first, it is set to the second value. ------------- Changes requested by kcr (Author). PR: https://git.openjdk.java.net/jdk/pull/3473
On Tue, 13 Apr 2021 21:10:56 GMT, Kevin Rushforth <kcr@openjdk.org> wrote:
Are you sure you need an `AtomicReference` to set the exception? I don't see any use of multiple threads, but I might be missing something. If you do need it, you need to fix the order of arguments.
`postVisitDirectory()` and `visitFile()` methods can be potentially called concurrently by `walkFileTree()`. Javadoc https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/nio/file/F...) doesn't say anything about synchronization, so I think it is fair to assume it is not guaranteed. ------------- PR: https://git.openjdk.java.net/jdk/pull/3473
On Wed, 14 Apr 2021 17:36:21 GMT, Alexey Semenyuk <asemenyuk@openjdk.org> wrote:
postVisitDirectory() and visitFile() methods can be potentially called concurrently by walkFileTree()
I don't think they can.
Javadoc ... doesn't say anything about synchronization, so I think it is fair to assume it is not guaranteed.
Given that the `Files` class says nothing about running its various file walking operations in parallel, I think you can be confident that the visitor methods are only ever run on the same thread that calls walkFileTree. I would consider it a bug if the thread used to callback into the visitor were different from the calling thread. Still, I think using `AtomicReference` is fine here, so this is a moot point for this PR. ------------- PR: https://git.openjdk.java.net/jdk/pull/3473
two changes: One to jpackage, when recursively removing directory, when IOException occurs, record it and continue (deleting as much as possible) before throwing the exception. The other to tests, when running jpackage via ProcessBuilder.execute(), set the "TMP" environment variable to the current value of System Property "java.io.tmpdir". This causes the sub-process (jpackage) to output tmp files to the tmp file location used by the test. (So the test harness can clean up after test exits).
Andy Herrick has updated the pull request incrementally with one additional commit since the last revision: JDK-8265078: jpackage tests on Windows leave large temp files ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/3473/files - new: https://git.openjdk.java.net/jdk/pull/3473/files/e78442ba..4b9e59f6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3473&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3473&range=01-02 Stats: 4 lines in 2 files changed: 2 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/3473.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3473/head:pull/3473 PR: https://git.openjdk.java.net/jdk/pull/3473
On Tue, 13 Apr 2021 22:50:12 GMT, Andy Herrick <herrick@openjdk.org> wrote:
two changes: One to jpackage, when recursively removing directory, when IOException occurs, record it and continue (deleting as much as possible) before throwing the exception. The other to tests, when running jpackage via ProcessBuilder.execute(), set the "TMP" environment variable to the current value of System Property "java.io.tmpdir". This causes the sub-process (jpackage) to output tmp files to the tmp file location used by the test. (So the test harness can clean up after test exits).
Andy Herrick has updated the pull request incrementally with one additional commit since the last revision:
JDK-8265078: jpackage tests on Windows leave large temp files
This looks fine. I see that this bug is listed as Windows-specific. Are any similar changes needed for other platforms, or do they already write test files only to `java.io.tmpdir`? ------------- Marked as reviewed by kcr (Author). PR: https://git.openjdk.java.net/jdk/pull/3473
On Tue, 13 Apr 2021 22:50:12 GMT, Andy Herrick <herrick@openjdk.org> wrote:
two changes: One to jpackage, when recursively removing directory, when IOException occurs, record it and continue (deleting as much as possible) before throwing the exception. The other to tests, when running jpackage via ProcessBuilder.execute(), set the "TMP" environment variable to the current value of System Property "java.io.tmpdir". This causes the sub-process (jpackage) to output tmp files to the tmp file location used by the test. (So the test harness can clean up after test exits).
Andy Herrick has updated the pull request incrementally with one additional commit since the last revision:
JDK-8265078: jpackage tests on Windows leave large temp files
Although jpackage failing to clean up has only been seen as an issue on Windows, allowing it to write outside the test directory (in default tmp dir) still exists on mac and linux - I will look into enhancing (what is now setWindowsTmpDir()) to be usefull on all platforms. ------------- PR: https://git.openjdk.java.net/jdk/pull/3473
On Tue, 13 Apr 2021 22:50:12 GMT, Andy Herrick <herrick@openjdk.org> wrote:
two changes: One to jpackage, when recursively removing directory, when IOException occurs, record it and continue (deleting as much as possible) before throwing the exception. The other to tests, when running jpackage via ProcessBuilder.execute(), set the "TMP" environment variable to the current value of System Property "java.io.tmpdir". This causes the sub-process (jpackage) to output tmp files to the tmp file location used by the test. (So the test harness can clean up after test exits).
Andy Herrick has updated the pull request incrementally with one additional commit since the last revision:
JDK-8265078: jpackage tests on Windows leave large temp files
Marked as reviewed by asemenyuk (Reviewer). ------------- PR: https://git.openjdk.java.net/jdk/pull/3473
On Tue, 13 Apr 2021 18:57:20 GMT, Andy Herrick <herrick@openjdk.org> wrote:
two changes: One to jpackage, when recursively removing directory, when IOException occurs, record it and continue (deleting as much as possible) before throwing the exception. The other to tests, when running jpackage via ProcessBuilder.execute(), set the "TMP" environment variable to the current value of System Property "java.io.tmpdir". This causes the sub-process (jpackage) to output tmp files to the tmp file location used by the test. (So the test harness can clean up after test exits).
This pull request has now been integrated. Changeset: e1675778 Author: Andy Herrick <herrick@openjdk.org> URL: https://git.openjdk.java.net/jdk/commit/e1675778 Stats: 30 lines in 3 files changed: 28 ins; 0 del; 2 mod 8265078: jpackage tests on Windows leave large temp files Reviewed-by: asemenyuk, kcr ------------- PR: https://git.openjdk.java.net/jdk/pull/3473
participants (3)
-
Alexey Semenyuk
-
Andy Herrick
-
Kevin Rushforth