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

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

Reply via email to