RFR: 8359477: com/sun/net/httpserver/Test12.java appears to have a temp file race [v4]

[Jaikiran Pai]

On Tue, 1 Jul 2025 09:31:15 GMT, Jaikiran Pai <jpai at 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

The PR is now ready for review. When I opened this PR a few weeks back I had removed the calls to `Files.delete(...)` in order to not introduce bigger changes to the test to co-ordinate the file deletion. However, Volkan's suggested approach of calling `ExecutorService.close()` to ensure that the server side handlers complete execution before deleting the files is a clean and straightforward way to address this issue. So i have re-introduced the Files.delete(...) calls and replaced the use of `ExecutorService.shutdown()` with `ExecutorService.close()` in the test. That's the real fix in this PR, the rest of the changes are clean ups to use newer constructs like try-with-resources.

With the current changes in this PR, I have run the test around a 1000 times and haven't seen it fail. Plus, I've also run the test with an aritifcial slowdown in the `FileServerHandler` and the test itself and I could reproduce the issue consistently without the proposed changes. It no longer reproduces even with artificial delays after the proposed changes.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/25820#issuecomment-3022992439