-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improved locking for HttpReceiver.ContentSource. #9007
Improved locking for HttpReceiver.ContentSource. #9007
Conversation
Improved response failure code path. Now either responseFailure() must be called, or exchange.responseComplete() followed by HttpReceiver.abort(). Fixed failAndClose() for HTTP/2 and HTTP/3: the connection must not be closed, stream.reset() is sufficient. Fixed flaky test HttpClientDemandTest.testTwoListenersWithDifferentDemand(). Fixed DistributionTests.testVirtualThreadPool(). Signed-off-by: Simone Bordet <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either take semantic is needed or better comment to explain why not.
invoker.run(() -> | ||
LOG.debug("Failing with {} on {}", failure, this); | ||
|
||
HttpExchange exchange = getHttpExchange(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the comment/test below, it feels like this should be a takeHttpExchange()
semantic, or at least the test below could be:
if (exchange == null || !disassociate(exchange))
At the very least, can the comment below explain how the race of exchange becoming null just after the get is handled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gregw I don't understand.
The semantic cannot be "take", as the "take" happens later when also the request is completed.
The call exchange.responseComplete()
is atomic, so the decision of whether doing something or not is taken there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbordet It is a general pattern for a race to have code in an async environment that does non atomic test and then act. It may well be that the exchange is atomically "taken" and nulled elsewhere, but a thread here may still have tested the exchange whilst non null and then act on it whilst it is null.
So either this is a race here, or there is some external circumstance that ensures that the responseComplete
can never be called between the test and the action. So we need to either fix the race or document why it is not a race.
jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpReceiver.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpReceiver.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpReceiver.java
Show resolved
Hide resolved
jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpReceiver.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpReceiver.java
Show resolved
Hide resolved
Signed-off-by: Simone Bordet <[email protected]>
Improved response failure code path.
Now either responseFailure() must be called, or exchange.responseComplete() followed by HttpReceiver.abort().
Fixed failAndClose() for HTTP/2 and HTTP/3: the connection must not be closed, stream.reset() is sufficient.
Fixed flaky test HttpClientDemandTest.testTwoListenersWithDifferentDemand().
Fixed DistributionTests.testVirtualThreadPool().
Signed-off-by: Simone Bordet [email protected]