Fixes #12266 - InvocationType improvements and cleanups. #12273
Closed
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
* Removed usages of `AbstractConnection.getInvocationType()`. * Changed HTTP server-side Connection implementations to use `AbstractConnection.fillInterested(Callback)` with a callback that specifies the `InvocationType`, derived from the `Server`, which derives it from the `Handler` chain. * Changed client-side receivers to use `AbstractConnection.fillInterested(Callback)` with a callback that specifies the `InvocationType`, derived from the `HttpClientTransport`. * Introduced `HttpClientTransport.getInvocationType(Connection)`, so that client applications can specify whether they block or not. * Made sure client-side HTTP/2 and HTTP/3 return tasks with the proper `InvocationType` to be run by the `ExecutionStrategy` when calling application code. * HTTP3StreamConnection now uses an `EITHER` fillable callback to possibly process streams in parallel. * `QuicStreamEndPoint` now uses a task to invoke `FillInterest.fillable()`, rather than invoking it directly, therefore honoring the `InvocationType` of callback set by the `Connection` associated with the `QuicStreamEndPoint`. Signed-off-by: Simone Bordet <[email protected]>
gregw
requested changes
Sep 15, 2024
| @@ -139,10 +137,21 @@ protected void failedCallback(final Callback callback, final Throwable x) | |||
| * @see #onFillable() | |||
| */ | |||
| public void fillInterested() | |||
| { | |||
There was a problem hiding this comment.
Should this method be deprecated now, so that we can eventually remove it?
| * <p>Registers read interest with the given callback.</p> | ||
| * <p>When read readiness is signaled, the callback will be completed.</p> | ||
| * | ||
| * @param callback the callback to complete when read readiness is signaled |
There was a problem hiding this comment.
Suggested change
| * @param callback the callback to complete when read readiness is signaled | |
| * @param callback the callback to complete when read readiness is signaled or there is a failure |
| @@ -88,6 +88,7 @@ public class HttpConnection extends AbstractMetaDataConnection implements Runnab | |||
| private static final ThreadLocal<HttpConnection> __currentConnection = new ThreadLocal<>(); | |||
| private static final AtomicLong __connectionIdGenerator = new AtomicLong(); | |||
|
|
|||
| private final Callback fillableCallback = new FillableCallback(); | |||
There was a problem hiding this comment.
Please match the naming style of the class
Suggested change
| private final Callback fillableCallback = new FillableCallback(); | |
| private final Callback _fillableCallback = new FillableCallback(); |
Comment on lines
+64
to
+70
| * <p>Invoking the task does not block the invoker thread, | ||
| * and the invocation may be performed immediately in the invoker thread.</p> | ||
| * <p>The thread that produced the task may dispatch another | ||
| * thread to resume production, and then invoke the task, differently | ||
| * from {@link #NON_BLOCKING} which does not dispatch production to | ||
| * another thread.</p> | ||
| * <p>The invocation cannot be deferred to a later time, differently |
There was a problem hiding this comment.
Suggested change
| * <p>Invoking the task does not block the invoker thread, | |
| * and the invocation may be performed immediately in the invoker thread.</p> | |
| * <p>The thread that produced the task may dispatch another | |
| * thread to resume production, and then invoke the task, differently | |
| * from {@link #NON_BLOCKING} which does not dispatch production to | |
| * another thread.</p> | |
| * <p>The invocation cannot be deferred to a later time, differently | |
| * <p>Invoking the {@link Invocable} may block the invoker thread, unless invoked | |
| * via {@link #invokeNonBlocking(Runnable)}, in which case it may not block the invoking thread. | |
| * The invocation cannot be deferred to a later time, differently |
Comment on lines
+72
to
+78
| * <p>A series of {@code NON_BLOCKING} tasks is run sequentially, | ||
| * while a series of {@code EITHER} tasks may be run in parallel, | ||
| * if there are threads available to resume task production.</p> | ||
| * <p>This invocation type is suitable for tasks that | ||
| * perform the non-deferrable action in a non-blocking way, | ||
| * hinting that may be run in parallel, for example when each task | ||
| * processes a different connection or a different stream.</p> |
There was a problem hiding this comment.
Suggested change
| * <p>A series of {@code NON_BLOCKING} tasks is run sequentially, | |
| * while a series of {@code EITHER} tasks may be run in parallel, | |
| * if there are threads available to resume task production.</p> | |
| * <p>This invocation type is suitable for tasks that | |
| * perform the non-deferrable action in a non-blocking way, | |
| * hinting that may be run in parallel, for example when each task | |
| * processes a different connection or a different stream.</p> | |
| * <p>This invocation type is suitable for tasks that | |
| * perform the non-deferrable action in a non-blocking way, | |
| * hinting that may be run in parallel, for example when each task | |
| * processes a different connection or a different stream.</p> | |
| * <p>For multiple {@code EITHER} tasks, if there threads available, then | |
| * those tasks may be run in parallel as if they are {@code BLOCKING} tasks, otherwise | |
| * they should be invoked serially via {@link #invokeNonBlocking(Runnable)} so | |
| * they act as {@code NON_BLOCKING}.</p> | |
| ` |
lorban
reviewed
Sep 16, 2024
| 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.
You could await until queue contains "-headers" instead of relying on time.
|
Closing in favor of #12299. |
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.
AbstractConnection.getInvocationType().AbstractConnection.fillInterested(Callback)with a callback that specifies theInvocationType, derived from theServer, which derives it from theHandlerchain.AbstractConnection.fillInterested(Callback)with a callback that specifies theInvocationType, derived from theHttpClientTransport.HttpClientTransport.getInvocationType(Connection), so that client applications can specify whether they block or not.InvocationTypeto be run by theExecutionStrategywhen calling application code.EITHERfillable callback to possibly process streams in parallel.QuicStreamEndPointnow uses a task to invokeFillInterest.fillable(), rather than invoking it directly, therefore honoring theInvocationTypeof callback set by theConnectionassociated with theQuicStreamEndPoint.