Skip to content
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

InvocationType improvements and cleanups #12266

Open
sbordet opened this issue Sep 12, 2024 · 0 comments · Fixed by #12551 · May be fixed by #12596 or #12299
Open

InvocationType improvements and cleanups #12266

sbordet opened this issue Sep 12, 2024 · 0 comments · Fixed by #12551 · May be fixed by #12596 or #12299
Assignees
Labels
Bug For general bugs on Jetty side

Comments

@sbordet
Copy link
Contributor

sbordet commented Sep 12, 2024

Jetty version(s)
12

Description
On the server, the InvocationType of a Handler controls how Jetty invokes the Handler chain.

On the client, instead, the InvocationType is hardcoded to be BLOCKING for HTTP/1.1, a mix of undefined, BLOCKING and NON_BLOCKING for HTTP/2 and HTTP/3.

There should be an option on the client to allow applications to specify the invocation type, with consistent defaults across transports if not specified.

With the occasion, we should also get rid of the deprecated AbstractConnection.getInvocationType().

@sbordet sbordet added the Bug For general bugs on Jetty side label Sep 12, 2024
@sbordet sbordet self-assigned this Sep 12, 2024
@sbordet sbordet moved this to 🏗 In progress in Jetty 12.0.14 Sep 12, 2024
sbordet added a commit that referenced this issue Sep 13, 2024
* 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]>
@sbordet sbordet linked a pull request Sep 13, 2024 that will close this issue
sbordet added a commit that referenced this issue Sep 23, 2024
* 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]>
@sbordet sbordet removed this from Jetty 12.0.14 Sep 25, 2024
sbordet added a commit that referenced this issue Nov 19, 2024
This is the work for the server-side only, the client side will be done in another pull request.

Previously, AbstractConnection.getInvocationType() was called by AbstractConnection.ReadCallback, but it was deprecated and is now removed, along with all its overrides.

This mechanism is now replaced by using a specific Callback implementation for each AbstractConnection subclass.
For example, HttpConnection uses HttpConnection.FillableCallback that in turn asks the InvocationType to the Server, and therefore the `Handler` tree.

Restored synchronous code for ServerFCGIConnection.close(), ensuring super.close() is always called.
Ensuring that in HttpConnection.close(), super.close() is always called.

Signed-off-by: Simone Bordet <[email protected]>
sbordet added a commit that referenced this issue Nov 22, 2024
This is the work for the server-side only, the client side will be done in another pull request.

Previously, `AbstractConnection.getInvocationType()` was called by `AbstractConnection.ReadCallback`, but it was deprecated and is now removed, along with all its overrides.

This mechanism is now replaced by using a specific Callback implementation for each `AbstractConnection` subclass.
For example, `HttpConnection` uses `HttpConnection.FillableCallback` that in turn asks the `InvocationType` to the Server, and therefore the `Handler` tree.

Introduced `AbstractConnection.NonBlocking` for the cases where `onFillable()` is non-blocking.

Restored synchronous code for `ServerFCGIConnection.close()`, ensuring `super.close()` is always called.
Ensuring that in `HttpConnection.close()` `super.close()` is always called.

Fixed promise notification to avoid race between the task (writing an error response) and the promise (resetting the stream) in HTTP/2 and HTTP/3.

Signed-off-by: Simone Bordet <[email protected]>
Co-authored-by: Greg Wilkins <[email protected]>
sbordet added a commit that referenced this issue Nov 29, 2024
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]>
@sbordet sbordet linked a pull request Nov 29, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment