Issue #12266 - InvocationType improvements and cleanups. #12596
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
This is the work for the client-side. Now the `InvocationType` can be specified at the `HttpClientTransport` level, or at the `ClientConnectionFactory.Info` level (for the dynamic transport). Wired the `InvocationType` also for `Response.ContentSourceListener`, so that for applications that read response content using `Content.Source` and specify an `InvocationType` to `demand(Runnable)`, the implementation will honor it. Signed-off-by: Simone Bordet <[email protected]>
Simplified retrieval of InvocationType, now only available from the HttpClientTransport. Signed-off-by: Simone Bordet <[email protected]>
…case of content not yet arrived. Signed-off-by: Simone Bordet <[email protected]>
Signed-off-by: Simone Bordet <[email protected]>
Before, `Stream.Listener.onHeaders()` was assuming that the headers were processed synchronously. Now, `HttpReceiverOverHTTP2` process them asynchronously, with a task that declares an invocation type. This was causing a race between the task and the code present after the call to `onHeaders()`. Introduced `Stream.Listener.onHeaders(Stream, HeadersFrame, Callback)` to allow asynchronous processing of `HeadersFrame`s. Optimised `HttpReceiver.responseHeaders()` to avoid creating the `Content.Source` if there is no content. Fixed `IteratingCallback` and `HTTP2Flusher` to use `tryLock()` in `toString()` to avoid deadlocks. Signed-off-by: Simone Bordet <[email protected]>
…type-cleanups'. Signed-off-by: Simone Bordet <[email protected]>
Signed-off-by: Simone Bordet <[email protected]>
Signed-off-by: Simone Bordet <[email protected]>
Signed-off-by: Simone Bordet <[email protected]>
lorban
requested changes
Dec 4, 2024
| void setConnectionPoolFactory(ConnectionPool.Factory factory); | ||
|
|
||
| /** | ||
| * @return the {@link InvocationType} associated with this {@code HttpClientTransport}. |
There was a problem hiding this comment.
The javadoc should explain that the invocation type is going to be used to figure out how to execute the listeners.
| public InvocationType getInvocationType() | ||
| { | ||
| Runnable demandCallback = demandCallbackRef.get(); | ||
| if (demandCallback == null) |
There was a problem hiding this comment.
There should definitely be a comment explaining why we return the connection's invocation type when the demand callback is null, as that's quite a special trick for which I already forgot the details of.
| @@ -188,7 +188,7 @@ interface AsyncContentListener extends ContentSourceListener | |||
| @Override | |||
| default void onContentSource(Response response, Content.Source contentSource) | |||
| { | |||
| Runnable demandCallback = Invocable.from(Invocable.InvocationType.NON_BLOCKING, () -> onContentSource(response, contentSource)); | |||
| Runnable demandCallback = Invocable.from(Invocable.getInvocationType(contentSource), () -> onContentSource(response, contentSource)); | |||
There was a problem hiding this comment.
There should probably be a comment explaining why we use the invocation type of the content source: how this trick exactly works is easy to forget.
...tty-client/src/main/java/org/eclipse/jetty/client/transport/HttpClientTransportOverHTTP.java
Show resolved
Hide resolved
...ort/src/main/java/org/eclipse/jetty/http2/client/transport/HttpClientTransportOverHTTP2.java
Show resolved
Hide resolved
...ort/src/main/java/org/eclipse/jetty/http3/client/transport/HttpClientTransportOverHTTP3.java
Show resolved
Hide resolved
| @@ -116,4 +120,48 @@ public String toString() | |||
| sender, | |||
| receiver); | |||
| } | |||
|
|
|||
| private class Listener implements Stream.Client.Listener | |||
There was a problem hiding this comment.
The executions of the runnable desserve to be documented.
| boolean held = lock.isHeldByCurrentThread(); | ||
| return String.format("%s@%x{handling=%s, handled=%s, send=%s, completed=%s, request=%s}", | ||
| String held = lock.isHeldByCurrentThread() ? "" : "?"; | ||
| return String.format("%s@%x[%s:handling=%s,handled=%s,send=%s,completed=%s,request=%s]", |
There was a problem hiding this comment.
I think the : should be part of the held string.
| response.write(false, null, Callback.NOOP); | ||
| // Wait to force the client to invoke the content | ||
| // callback separately from the headers callback. | ||
| Thread.sleep(500); |
There was a problem hiding this comment.
No sleep please; it's better to wait until the headers event has been called.
| { | ||
| return String.format("%s@%x[%s, %b, %s]", getClass().getSimpleName(), hashCode(), _state, _aborted, _failure); | ||
| String held = _lock.isHeldByCurrentThread() ? "" : "?"; |
There was a problem hiding this comment.
: should be in the held string.
…type-cleanups'. Signed-off-by: Simone Bordet <[email protected]>
…type-cleanups'. Signed-off-by: Simone Bordet <[email protected]>
…type-cleanups'. Signed-off-by: Simone Bordet <[email protected]>
Fixed reset race in HTTP2Stream. Signed-off-by: Simone Bordet <[email protected]>
Signed-off-by: Simone Bordet <[email protected]>
Signed-off-by: Simone Bordet <[email protected]>
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.
This is the work for the client-side.
Now the
InvocationTypecan be specified at theHttpClientTransportlevel, or at theClientConnectionFactory.Infolevel (for the dynamic transport).Wired the
InvocationTypealso forResponse.ContentSourceListener, so that for applications that read response content usingContent.Sourceand specify anInvocationTypetodemand(Runnable), the implementation will honor it.