8304065: HttpServer.stop should terminate immediately if no exchanges are in progress#24467
8304065: HttpServer.stop should terminate immediately if no exchanges are in progress#24467eirbjo wants to merge 2 commits intoopenjdk:masterfrom
Conversation
|
👋 Welcome back eirbjo! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
eirbjo
force-pushed
the
httpserver-stop-termination
branch
from
6fb25d5 to
f06c8df
Compare
| long latest = System.nanoTime() + Duration.ofSeconds(delay).toNanos(); | ||
| while (activeExchanges() > 0 && System.nanoTime() < latest) { |
There was a problem hiding this comment.
I don't think that works unfortunately. We need something that takes into account that an exchange may have been started but hasn't reached the point where startExchange has been called.
The problem is that exchangeCount is incremeted asynchronously in the ExchangeImpl constructor, and the ExchangeImpl is created by the Exchange::run method - which is run asynchronoously in the executor (submitted by the dispacher thread).
Possibly posting a StopRequested event to the dispatcher thread, and have the dispacther thread switch finished = true when is sees the StopRequested event and notices that allConnections only contains idleConnections (in which case we could also assert that exchangeCount == 0).
Unless I'm not mistaken this is not going to be a trivial fix.
There was a problem hiding this comment.
Unless I'm not mistaken this is not going to be a trivial fix.
Alright, I'll convert this to a draft PR for now. I may explore solutions, but it seems a full fix could be above my pay grade.
| long latest = System.nanoTime() + Duration.ofSeconds(delay).toNanos(); | ||
| while (activeExchanges() > 0 && System.nanoTime() < latest) { |
There was a problem hiding this comment.
Also the arithmetic is dubious since System.nanoTime() + Duration.ofSeconds(delay).toNanos(); could overflow. You never want to compare val1 < val2 - use val2 - val1 > 0 instead.
@eirbjo: @myankelev has a fix for this issue and will be taking over where you left off. I believe he is reusing the test you provided and will add you as a co-author of the new PR. I hope this is OK with you! |
@dfuch Absolutely no worries at all! |
|
@eirbjo this pull request can not be integrated into git checkout httpserver-stop-termination
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
openjdk
bot
added
the
merge-conflict
3 participants
Please help review this PR which improves
HttpServer::stoptermination timing to better fit user expectations.This PR:
ServerImpl::stopcontinue without delay when there are no active exchanges during shutdownAdditionally,
ServerImpl::stopis updated to useSystem::nanotimeinstead ofSystem::currentTimeMilliswhen calculating wait times.The test relies on timing to assert the order of events. Not perfect, but it seems to work.
(This part of the code base is rather new to me. A bit of hand-holding should be expected when reviewing this PR)
Progress
Warnings
Issue
Backport <hash>with the hash of the original commit. See Backports.Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24467/head:pull/24467$ git checkout pull/24467Update a local copy of the PR:
$ git checkout pull/24467$ git pull https://git.openjdk.org/jdk.git pull/24467/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 24467View PR using the GUI difftool:
$ git pr show -t 24467Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24467.diff
Using Webrev
Link to Webrev Comment