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.

Sign up for GitHub

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

Jump to bottom

InvocationType improvements and cleanups #12266

Closed
sbordet opened this issue Sep 12, 2024 · 1 comment · Fixed by #12551 or #12596
Closed

InvocationType improvements and cleanups #12266

sbordet opened this issue Sep 12, 2024 · 1 comment · Fixed by #12551 or #12596
Assignees
@sbordet
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
@sbordet
Fixes #12266 - InvocationType improvements and cleanups.
e911c23 
* 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
Fixes #12266 - InvocationType improvements and cleanups. #12273
Closed
@sbordet sbordet mentioned this issue Sep 17, 2024
Merged
sbordet added a commit that referenced this issue Sep 23, 2024
@sbordet
Fixes #12266 - InvocationType improvements and cleanups.
b08b57e 
* 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
This was referenced Oct 2, 2024
Open
Closed
sbordet added a commit that referenced this issue Nov 19, 2024
@sbordet
Issue #12266 - InvocationType improvements and cleanups.
8e7ce53 
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]>
This was linked to pull requests Nov 19, 2024
Fixes #12266 - InvocationType improvements and cleanups. #12299
Closed
Issue #12266 - InvocationType improvements and cleanups. #12551
Merged
sbordet added a commit that referenced this issue Nov 22, 2024
@sbordet @gregw
Issue #12266 - InvocationType improvements and cleanups. (#12551)
3d68e2b 
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
@sbordet
Issue #12266 - InvocationType improvements and cleanups.
1345a6f 
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
Issue #12266 - InvocationType improvements and cleanups. #12596
Merged
sbordet added a commit that referenced this issue Jan 7, 2025
@sbordet
Issue #12266 - InvocationType improvements and cleanups. (#12596)
4939a8b 
* This is the work for the client-side.
  Now the `InvocationType` can be specified at the `HttpClientTransport` level.
  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.
* Renamed HttpClient.getTransport() to getHttpClientTransport().
* Fixed FCGI parsing: onResponseHeaders() was called multiple times in case of content not yet arrived.
* Fixed race condition when notifying HTTP/2 `HeadersFrame`s.
  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.
* Fixed `IteratingCallback` and `HTTP2Flusher` to use `tryLock()` in `toString()` to avoid deadlocks.
* Fixed HTTP/2 serialization in HttpReceiverOverHTTP2. Fixed reset race in HTTP2Stream.
* Fixed handling of HTTP upgrade in CoreClientUpgradeRequest.
  For HTTP/2, the response may have arrived, but the exchange failed (due to the request being failed); in this case, we want to notify an UpgradeException, not a generic Exception.

Signed-off-by: Simone Bordet <[email protected]>
@sbordet
Copy link
Contributor Author

sbordet commented Jan 15, 2025

Done in #12551 and #12596.

@sbordet sbordet closed this as completed Jan 15, 2025
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Jetty 12.1.0 Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sbordet sbordet

Labels
Bug For general bugs on Jetty side
Projects
Jetty 12.1.0
Status: ✅ Done
Milestone
No milestone
1 participant
@sbordet