8304065: HttpServer.stop should terminate immediately if no exchanges are in progress by myankelev · Pull Request #25333 · openjdk/jdk

myankelev · 2025-05-20T15:51:50Z

HttpServer::stop will terminate the server immidiately after all exhcnages are complete.
If the exchanges take longer then the specified delay it will terminate straight after the delay, the same as the previous behaviour.

Used to wait until the delay is complete at all times, regardless of the number of active exchanges.

Tests based on @eirbjo work, so adding Eirik as a contributor.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8304065: HttpServer.stop should terminate immediately if no exchanges are in progress (Bug - P4)

Reviewers

Daniel Fuchs (@dfuch - Reviewer)
Michael McMahon (@Michael-Mc-Mahon - Reviewer)

Contributors

Eirik Bjørsnøs <eirbjo@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25333/head:pull/25333
$ git checkout pull/25333

Update a local copy of the PR:
$ git checkout pull/25333
$ git pull https://git.openjdk.org/jdk.git pull/25333/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25333

View PR using the GUI difftool:
$ git pr show -t 25333

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25333.diff

Using Webrev

Link to Webrev Comment

…nges are in progress HttpServer::stop will terminate the server immidiately after all exhcnages are complete. If the exchanges take longer then the specified delay it will terminate straight after the delay. Used to wait until the delay is complete at all times, regardless of the number of active exchanges.

bridgekeeper · 2025-05-20T15:52:27Z

👋 Welcome back myankelevich! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-05-20T15:53:34Z

@myankelev This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8304065: HttpServer.stop should terminate immediately if no exchanges are in progress

Co-authored-by: Eirik Bjørsnøs <eirbjo@openjdk.org>
Reviewed-by: dfuchs, michaelm

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 256 new commits pushed to the master branch:

39714b6: 8357869: Remove PreviewNote taglet in its current form
4e1878c: 8355022: Implement JEP 506: Scoped Values
f02190b: 8357829: Commented out sample limit in JfrSamplerThread::task_stacktrace
... and 253 more: https://git.openjdk.org/jdk/compare/d930dc5df376268d3d96ac5dcddc47496459f5c0...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@dfuch, @Michael-Mc-Mahon) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

myankelev · 2025-05-20T15:53:47Z

/contributor add @eirbjo

openjdk · 2025-05-20T15:54:24Z

@myankelev
Contributor Eirik Bjørsnøs <eirbjo@openjdk.org> successfully added.

openjdk · 2025-05-20T15:54:56Z

@myankelev The following label will be automatically applied to this pull request:

net

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-05-20T15:58:40Z

Webrevs

dfuch

Some early comments. I will review the test next.

src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java

src/jdk.httpserver/share/classes/sun/net/httpserver/StopRequestedEvent.java

src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java

test/jdk/com/sun/net/httpserver/ServerStopTerminationTest.java

dfuch · 2025-05-20T17:11:45Z

test/jdk/com/sun/net/httpserver/ServerStopTerminationTest.java

+        final Duration delayDuration = Duration.ofSeconds(1);
+        log("Shutting down the server the first time");
+        timeShutdown(delayDuration);
+        log("Shutting down the server the second time");
+        timeShutdown(delayDuration);


Should we check the time it took the server to shutdown here too?

This test just validates, that there is no error thrown. The other tests cover the timing scenarios

test/jdk/com/sun/net/httpserver/ServerStopTerminationTest.java

dfuch · 2025-05-20T18:11:47Z

Looking at Dispatcher::run - there are a couple of additional places where we should take into account the finished flag.
I wonder if we should stop processing events as soon as the finished has been set by one event. That would mean testing for finished right after calling handleEvent - or change handleEvent to return a boolean, and exit out of the while loop and close the selector if we have set finished.
Arguably - maybe there should not be any event in the queue after finished has been set - but there doesn't seem any valid reason to reRegister connections or call select(1000) again at that point.

src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java

openjdk · 2025-05-21T16:46:33Z

@myankelev Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java

test/jdk/com/sun/net/httpserver/ServerStopTerminationTest.java

Co-authored-by: Daniel Fuchs <67001856+dfuch@users.noreply.github.com>

msheppar · 2025-05-23T08:48:32Z

If I have read the original code correctly there is a flaw in it, that is the startExchange is never called.
Thus the code in handleEvent won’t executed as anticipated

391 int exchanges = endExchange();
392 if (terminating && exchanges == 0) {
393 finished = true;
394 }

Exchanges will always be negative. This is inherited in the new update

WRT current PR, I don't think it is good that the state of the server is encapsulated in the state of the CountDownLatch.
The use of the CountDownLatch to synchronise the stop and the Dispatcher shutdown is fine, but having the server state encapsulated in the CountDownLatch value is a bit oblique or obfuscating. I think the original volatile boolean variable is a clearer semantics

I would make stop synchronised to have only one thread executing at time. First check on entry then becomes test on the server's current state. This means that this state management needs some attention.

The addition of Stop event is a neat idea, but draws attention to the semantics of the stop. What is the desired semantics of stop, to not accept any new connections, i.e. stop the listener immediately, not accept new requests on existing connections. To allow existing Exchanges to complete and then shutdown all extant connections.

msheppar · 2025-05-23T08:50:11Z

src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java

@@ -223,44 +225,69 @@ public HttpsConfigurator getHttpsConfigurator () {
    }

    public final boolean isFinishing() {


encapsulating the server state in a CountDownLatch is obfuscated semantics

Yes, but this is an existing function and it is used in other classes. I didn't change the functionality

the internal state representation of the server could do with a rethink and overhaul, and representing part of that state in a synchronisation primitive obscures/obfuscates the server state's semantics. I don't think it's "clean code"

I agree that the semantic of finished finishing is a bit obfuscated. On the other hand I don't think we should go so far as overhauling / rethinking the states in this PR. If you recall there have been lots of fixes in this area (we had some assert firing at some point, and it took some time to find the root cause and fix).
That said having both a boolean and a countdown latch would be duplicate info, and potentially harder to manage consistency.
If we don't want the countdown latch we might go back to looping with Thread.sleep in stop, or use wait/notify, and I am not sure it would be much better.

I have a suggestion. Let's do this:

public final boolean finished() { // we're finished when the latch reaches 0 return finishedLatch.getCount() == 0; } public final boolean isFinishing() { return finished(); }

Then replace things like if (finished ... with if (finished() ...
That should minimize the changes and make them look less awkward.

msheppar · 2025-05-23T08:55:10Z

src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java

+     * @param delay maximum delay to wait for exchanges completion, in seconds
+     */
    public void stop (int delay) {
        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)

msheppar · 2025-05-23T08:59:45Z

src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java

                if (r instanceof WriteFinishedEvent) {

                    logger.log(Level.TRACE, "Write Finished");
                    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.

msheppar · 2025-05-23T09:00:39Z

src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java

+     * complete within the imparted delay.
+     *
+     * @param delay maximum delay to wait for exchanges completion, in seconds
+     */


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.

dfuch · 2025-05-23T09:57:02Z

If I have read the original code correctly there is a flaw in it, that is the startExchange is never called.

@msheppar what do you mean by that? It's called in ExchangeImpl.java

msheppar · 2025-05-23T13:32:56Z

If I have read the original code correctly there is a flaw in it, that is the startExchange is never called.

@msheppar what do you mean by that? It's called in ExchangeImpl.java

Well then I haven't read it correctly :-( needs a deeper dive, I didn't examine the Exchange::run method in sufficient detail ... thanks for the correction .... BUT in my defence I see Dispatcher invoke handle, handle creates an Exchange, which is dispatched to an Executor and I would have expected that the run method of Exchange to call startExchange, as the log message says an exchange has started !!

Michael-Mc-Mahon

I wonder could the two Event types be consolidated inside Event.java? It seems overkill to have them in three separate source files now.

dfuch · 2025-05-26T16:28:52Z

I wonder could the two Event types be consolidated inside Event.java? It seems overkill to have them in three separate source files now.

Do you mean refactoring WriteFinishEvent and StopRequestedEvent as inner classes of Event.java?

Michael-Mc-Mahon · 2025-05-27T13:04:38Z

I wonder could the two Event types be consolidated inside Event.java? It seems overkill to have them in three separate source files now.

Do you mean refactoring WriteFinishEvent and StopRequestedEvent as inner classes of Event.java?

Potentially, but I thought the simplest approach would be as top-level package private classes in Event.java

msheppar · 2025-05-27T13:35:12Z

src/jdk.httpserver/share/classes/sun/net/httpserver/Event.java

    protected Event (ExchangeImpl t) {
        this.exchange = t;
    }
+


I don't see the purpose of this change and the value it brings other than collation and reducing explicit files for the derived Events ?
Could you elaborate the rationale please?

I do see a code smell in WriteFinished directly access the state of ExchangeImpl

Michael-Mc-Mahon · 2025-05-27T13:37:15Z

If I have read the original code correctly there is a flaw in it, that is the startExchange is never called. Thus the code in handleEvent won’t executed as anticipated

391 int exchanges = endExchange(); 392 if (terminating && exchanges == 0) { 393 finished = true; 394 }

Exchanges will always be negative. This is inherited in the new update

WRT current PR, I don't think it is good that the state of the server is encapsulated in the state of the CountDownLatch. The use of the CountDownLatch to synchronise the stop and the Dispatcher shutdown is fine, but having the server state encapsulated in the CountDownLatch value is a bit oblique or obfuscating. I think the original volatile boolean variable is a clearer semantics

I would make stop synchronised to have only one thread executing at time. First check on entry then becomes test on the server's current state. This means that this state management needs some attention.

The addition of Stop event is a neat idea, but draws attention to the semantics of the stop. What is the desired semantics of stop, to not accept any new connections, i.e. stop the listener immediately, not accept new requests on existing connections. To allow existing Exchanges to complete and then shutdown all extant connections.

startExchange() is called in the constructor for ExchangeImpl, which is not changing in this PR.

src/jdk.httpserver/share/classes/sun/net/httpserver/Event.java

src/jdk.httpserver/share/classes/sun/net/httpserver/FixedLengthOutputStream.java

src/jdk.httpserver/share/classes/sun/net/httpserver/UndefLengthOutputStream.java

dfuch

LGTM

myankelev · 2025-05-28T10:00:22Z

@dfuch, @msheppar, @djelinski , @Michael-Mc-Mahon, thank you for your reviews!

myankelev · 2025-05-28T10:00:29Z

/integrate

openjdk · 2025-05-28T10:00:56Z

@myankelev
Your change (at version 1ca9ecd) is now ready to be sponsored by a Committer.

Michael-Mc-Mahon

Looks good

Michael-Mc-Mahon · 2025-05-28T10:34:39Z

/sponsor

openjdk · 2025-05-28T10:34:53Z

Going to push as commit 627ef34.
Since your change was applied there have been 256 commits pushed to the master branch:

39714b6: 8357869: Remove PreviewNote taglet in its current form
4e1878c: 8355022: Implement JEP 506: Scoped Values
f02190b: 8357829: Commented out sample limit in JfrSamplerThread::task_stacktrace
... and 253 more: https://git.openjdk.org/jdk/compare/d930dc5df376268d3d96ac5dcddc47496459f5c0...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-05-28T10:35:01Z

@Michael-Mc-Mahon @myankelev Pushed as commit 627ef34.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

myankelev added 2 commits May 20, 2025 11:30

new line in the end of the test

51fffc9

openjdk bot added the rfr Pull request is ready for review label May 20, 2025

openjdk bot added the net net-dev@openjdk.org label May 20, 2025

dfuch reviewed May 20, 2025

View reviewed changes

src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java Outdated Show resolved Hide resolved

dfuch reviewed May 20, 2025

View reviewed changes

src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java Show resolved Hide resolved

dfuch reviewed May 20, 2025

View reviewed changes

djelinski reviewed May 21, 2025

View reviewed changes

src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java Outdated Show resolved Hide resolved

src/jdk.httpserver/share/classes/sun/net/httpserver/ServerImpl.java Outdated Show resolved Hide resolved

myankelev added 2 commits May 21, 2025 17:30

addressing comments

c924f79

cleanup

54def0d

myankelev force-pushed the JDK-8304065 branch from f8a9148 to 54def0d Compare May 21, 2025 16:46

dfuch reviewed May 22, 2025

View reviewed changes

Suggestions from cr

5a87910

Co-authored-by: Daniel Fuchs <67001856+dfuch@users.noreply.github.com>

msheppar reviewed May 23, 2025

View reviewed changes

Michael-Mc-Mahon reviewed May 26, 2025

View reviewed changes

consolidated events

50749ea

msheppar reviewed May 27, 2025

View reviewed changes

dfuch reviewed May 27, 2025

View reviewed changes

src/jdk.httpserver/share/classes/sun/net/httpserver/Event.java Outdated Show resolved Hide resolved

src/jdk.httpserver/share/classes/sun/net/httpserver/Event.java Outdated Show resolved Hide resolved

dfuch reviewed May 27, 2025

View reviewed changes

src/jdk.httpserver/share/classes/sun/net/httpserver/Event.java Outdated Show resolved Hide resolved

dfuch reviewed May 27, 2025

View reviewed changes

src/jdk.httpserver/share/classes/sun/net/httpserver/Event.java Show resolved Hide resolved

dfuch reviewed May 27, 2025

View reviewed changes

src/jdk.httpserver/share/classes/sun/net/httpserver/FixedLengthOutputStream.java Outdated Show resolved Hide resolved

dfuch reviewed May 27, 2025

View reviewed changes

src/jdk.httpserver/share/classes/sun/net/httpserver/UndefLengthOutputStream.java Outdated Show resolved Hide resolved

discussion

1ca9ecd

dfuch approved these changes May 28, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 28, 2025

openjdk bot added the sponsor Pull request is ready to be sponsored label May 28, 2025

Michael-Mc-Mahon approved these changes May 28, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label May 28, 2025

openjdk bot closed this May 28, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels May 28, 2025

vy mentioned this pull request Jun 19, 2025

8304065: HttpServer.stop should terminate immediately if no exchanges are in progress #24467

Closed

3 tasks

		@@ -223,44 +225,69 @@ public HttpsConfigurator getHttpsConfigurator () {
		}

		public final boolean isFinishing() {

Conversation

myankelev commented May 20, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Contributors

Reviewing

Uh oh!

bridgekeeper bot commented May 20, 2025

Uh oh!

openjdk bot commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

myankelev commented May 20, 2025

Uh oh!

openjdk bot commented May 20, 2025

Uh oh!

openjdk bot commented May 20, 2025

Uh oh!

mlbridge bot commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dfuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dfuch commented May 20, 2025

Uh oh!

Uh oh!

Uh oh!

openjdk bot commented May 21, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

msheppar commented May 23, 2025

Uh oh!

msheppar May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfuch commented May 23, 2025

Uh oh!

msheppar commented May 23, 2025

myankelev commented May 20, 2025 •

edited by openjdk bot

Loading

openjdk bot commented May 20, 2025 •

edited

Loading

mlbridge bot commented May 20, 2025 •

edited

Loading

msheppar May 23, 2025 •

edited

Loading