On Tue, 1 Jul 2025 09:31:15 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> Can I please get a review of this test-only change which addresses an >> intermittent failure in `test/jdk/com/sun/net/httpserver/Test12.java`? This >> fixes https://bugs.openjdk.org/browse/JDK-8359477. >> >> In the linked JBS issue I've added a comment which explains what causes this >> intermittent failure. The issue is a bug/race within the test. There are 2 >> commits in this PR and only of them is necessary for addressing this issue. >> The other one is a general cleanup of the test. I am willing to revert the >> general cleanup commit from this test if others feel that it shouldn't be >> done in this PR. >> >> The actual fix is this commit >> https://github.com/openjdk/jdk/commit/a0f8fc806e1682ef909cb7b4a449be855072fe48 >> which stops deleting the input files upon test completion. That should be >> OK because the jtreg test infrastructure will clean them up from the scratch >> directory after the completion of the test run (if the test succeeds). There >> are other ways to address this race condition and continue to delete these >> files (for example, co-ordinating between the test and the test specific >> `FileServerHandler`), but to me it didn't seem worthwhile doing that. >> >> I've run this change around a 1000 times and haven't seen it fail. >> >> P.S: The `test/jdk/com/sun/net/httpserver/Test1.java` does the same things >> as this test, but does it sequentially. I'm pretty sure that we should be >> able to trigger this intermittent test failure in that test too if I reorder >> the `test()` calls in that test to have a fixed length test be the last one >> to execute (i.e. `test (true, ...)` instead of the current `test(false, >> ...)`). > > Jaikiran Pai has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains eight additional > commits since the last revision: > > - use ExecutorService.close() instead of ExecutorService.shutdown() to wait > for the server side handlers to complete execution > - use try-with-resource in test's FileServerHandler > - Revert "8359477: com/sun/net/httpserver/Test12.java appears to have a temp > file race" > > This reverts commit a0f8fc806e1682ef909cb7b4a449be855072fe48. > - merge latest master branch changes > - minor change to test's log message > - add a log message in the test > - 8359477: com/sun/net/httpserver/Test12.java appears to have a temp file > race > - 8359477: general cleanup of the test test/jdk/com/sun/net/httpserver/Test12.java line 66: > 64: Path smallFilePath = createTempFileOfSize(TEMP_FILE_PREFIX, null, > 23); > 65: Path largeFilePath = createTempFileOfSize(TEMP_FILE_PREFIX, null, > 2730088); > 66: try (final ExecutorService executor = > Executors.newCachedThreadPool()) { I would not use try-with-resources here: I don't think we should close the executor before closing the servers. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25820#discussion_r2177073217
