-
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
Make Client's Request and Response abort()
fully async
#8725
Merged
lorban
merged 5 commits into
jetty-12.0.x
from
jetty-12.0.x-review-client-request-abort
Oct 21, 2022
Merged
Make Client's Request and Response abort()
fully async
#8725
lorban
merged 5 commits into
jetty-12.0.x
from
jetty-12.0.x-review-client-request-abort
Oct 21, 2022
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… CompletableFuture<Boolean> Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
sbordet
requested changes
Oct 18, 2022
jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpChannel.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpDestination.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/util/FutureResponseListener.java
Outdated
Show resolved
Hide resolved
...t/src/main/java/org/eclipse/jetty/http2/client/transport/internal/HttpReceiverOverHTTP2.java
Outdated
Show resolved
Hide resolved
...ttp2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HTTP2ServerConnection.java
Outdated
Show resolved
Hide resolved
...ttp2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HTTP2ServerConnection.java
Show resolved
Hide resolved
...t/src/main/java/org/eclipse/jetty/http3/client/transport/internal/HttpReceiverOverHTTP3.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/api/Stream.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/api/Stream.java
Outdated
Show resolved
Hide resolved
gregw
requested changes
Oct 18, 2022
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.
Can you provide a description for the PR, perhaps suggesting which of the 40 files contains the core of the changes.
lorban
changed the title
Make Request and Response abort() fully async
Make Client's Request and Response Oct 19, 2022
abort()
fully async
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
sbordet
requested changes
Oct 21, 2022
jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpReceiver.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Ludovic Orban <[email protected]>
sbordet
approved these changes
Oct 21, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The client's
Request
andResponse
interfaces contain the following method:The contract of that
abort()
method is to asynchronously notify a running request that it has to be aborted.That
abort()
method is meant to run concurrently with the servicing of the request, meaning that the request may be aborted, or it may finish before the abort takes effect, and some actions must happen depending on the outcome of this race condition: eitherabort()
won the race and -for instance in H2- a reset needs to be sent, orabort()
lost the race and it basically must be treated as a no-op.The problem of the current method signature is that it returns a boolean that is returned immediately while that boolean is supposed to be computed during the async execution. This has hard implications on how
abort()
can be implemented, and prevents some refactorings of the client's internals.The solution is to change the contract such as the
boolean
is delivered asynchronously: