On Fri, 23 May 2025 09:00:39 GMT, Mark Sheppard <mshep...@openjdk.org> wrote:
>> Mikhail Yankelevich has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Suggestions from cr
>>
>> Co-authored-by: Daniel Fuchs <67001856+df...@users.noreply.github.com>
>
> src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java line 239:
>
>> 237: *
>> 238: * @param delay maximum delay to wait for exchanges completion, in
>> seconds
>> 239: */
>
> make synchronised to allow only one thread to execute stop at any one time
Not sure that's the best solution. But if the `terminating` flag is already set
then it means that another thread has initiated stop, and maybe in this case we
should just jump to waiting on the latch - no need to post the event again or
close the channel and wakeup the selector again. The part in `stop` that need
synchronization already have it, so I don't think declaring stop as
synchronized would be such a good idea. In particular it would prevent other
threads to call getExchange() / endExchange() so I would be afraid it would
prevent active exchanges from proceeding - unless we use wait/notify instead of
a countdown latch - but that might be even more difficult to get right.
> src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java line 241:
>
>> 239: */
>> 240: public void stop (int delay) {
>> 241: if (delay < 0) {
>
> with the use of CountDownLatch the negative check becomes superfluous i.e.
>
> public boolean await(long timeout, TimeUnit unit) throws InterruptedException
> Causes the current thread to wait until the latch has counted down to zero,
> unless the thread is interrupted, or the specified waiting time elapses.
>
> . . .
> If the specified waiting time elapses then the value false is returned. If
> the time is less than or equal to zero, the method will not wait at all.
It's not something we should change though, as it's part of the API contract in
`HttpServer::stop`:
https://docs.oracle.com/en/java/javase/24/docs/api/jdk.httpserver/com/sun/net/httpserver/HttpServer.html#stop(int)
> src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java line 437:
>
>> 435:
>> 436: logger.log(Level.TRACE, "Write Finished");
>> 437: int exchanges = endExchange();
>
> startExchange is never called => endExchange can return negative ?
I hope that this is a scenario that cannot happen: to end an exchange you would
have to start it first.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25333#discussion_r2104221149
PR Review Comment: https://git.openjdk.org/jdk/pull/25333#discussion_r2104204781
PR Review Comment: https://git.openjdk.org/jdk/pull/25333#discussion_r2104206439
