Fixes #12266 - InvocationType improvements and cleanups. #12299
Conversation
* 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]>
| } | ||
|
|
||
| @Override | ||
| public InvocationType getInvocationType() |
There was a problem hiding this comment.
Shouldn't the InvocationType be precomputed in the constructor?
| @Override | ||
| public InvocationType getInvocationType() | ||
| { | ||
| return getHttpDestination().getHttpClient().getTransport().getInvocationType(delegate); |
There was a problem hiding this comment.
Shouldn't the InvocationType be precomputed in the constructor?
| @Override | ||
| public InvocationType getInvocationType() | ||
| { | ||
| return getConnector().getServer().getInvocationType(); |
There was a problem hiding this comment.
Shouldn't the InvocationType be precomputed in the constructor?
| fillInterested(httpConnection); | ||
| } | ||
|
|
||
| private void fillInterested(HttpConnectionOverFCGI httpConnection) |
There was a problem hiding this comment.
I find this helper confusing, inlining it would be clearer IMHO.
| @@ -210,6 +212,11 @@ public void onOpen() | |||
| } | |||
| } | |||
|
|
|||
| void setFillInterest() | |||
There was a problem hiding this comment.
Now there is both fillInterested() and setFillInterest() methods, with no real explanation about which should be used when.
| { | ||
| @Override | ||
| public void onDataAvailable(Stream stream) | ||
| { | ||
| Stream.Data data = stream.readData(); | ||
| logger.info("onDataAvailable {}", data); |
There was a problem hiding this comment.
This should be logged at debug level.
| @@ -286,21 +286,31 @@ public boolean handle(Request request, Response response, Callback callback) thr | |||
| @Override | |||
| public void onHeaders(Stream stream, HeadersFrame frame) | |||
| { | |||
| System.err.println("SIMON: frame = " + frame); | |||
| } | ||
|
|
||
| @Override | ||
| public void onDataAvailable(Stream stream) | ||
| { | ||
| Stream.Data data = stream.readData(); | ||
| System.err.println("SIMON: data = " + data); |
| if (LOG.isDebugEnabled()) | ||
| LOG.debug("fillInterested {}", this); | ||
| getEndPoint().fillInterested(_readCallback); | ||
| fillInterested(_readCallback); |
There was a problem hiding this comment.
I'm growing a feeling that this method should be removed and be replaced by fillInterested(Callback callback).
| 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 that queue contains "*-headers" instead.
|
It seems |
|
@sbordet status of this? |
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.