On Tue, 1 Jul 2025 09:54:07 GMT, Daniel Fuchs <[email protected]> wrote:
>> 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.
I've updated the PR to close the Executor after the servers have been closed.
I'm guessing the suggestion was mostly as a precaution than what this test
currently does?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25820#discussion_r2177320230
