Cleanup DelayedDispatch handling. #9051 #12077
Conversation
|
@sbordet @lorban Your thoughts on doing this? If you think it is a good idea, can you contribute the h2 and h3 delayed dispatch handling to this branch. |
|
Implementation-wise, this looks alright assuming the same logic can be replicated in H2/H3 (but I doubt this is going to be troublesome). But could you summarize what benefits/drawbacks this has over the existing |
|
@lorban wrote:
The |
|
I've also taken the opportunity to cleanup lots of deprecation and TODOs in HttpConnection |
|
@lachlan-roberts Can you take over the handling of delayed multipart in this PR? |
…tty-12.1.x/delayedDispatch
Signed-off-by: Lachlan Roberts <[email protected]>
…ispatch' into experiment/jetty-12.1.x/delayedDispatch
jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java
Outdated
Show resolved
Hide resolved
…tty-12.1.x/delayedDispatch # Conflicts: # jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java # jetty-core/jetty-tests/jetty-test-client-transports/src/test/java/org/eclipse/jetty/test/client/transport/AbstractTest.java # jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletTest.java # jetty-ee11/jetty-ee11-servlet/src/test/java/org/eclipse/jetty/ee11/servlet/ServletTest.java # jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/ServletTest.java
jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/AbstractConnection.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/AbstractConnection.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java
Show resolved
Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java
Outdated
Show resolved
Hide resolved
jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ServletTest.java
Outdated
Show resolved
Hide resolved
jetty-ee11/jetty-ee11-servlet/src/test/java/org/eclipse/jetty/ee11/servlet/ServletTest.java
Outdated
Show resolved
Hide resolved
jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/ServletTest.java
Outdated
Show resolved
Hide resolved
|
@lorban I can't reproduce, but the CI leak looks real enough and is definitely associated with the code that is changed in this PR. |
|
@lorban actually, if you look at the full log of the CI failure, it is a write side buffer that is leaked, which is unchanged logic??? The reported releases are read side??????? |
|
@gregw the full log shows 3 leaks, as summed up by the first one is during write: second one during read: third one during write again: but it looks like a flake since it cannot be reproduced, so I assume these are pre-existing bugs that were not introduced by the current PR. Fortunately the buffer pool is leak-resistant, so this is merely an annoyance. |
|
@sbordet @lorban @lachlan-roberts assuming the leak was a flake, can I get a review to commit? |
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/HttpConnectionTest.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/DelayedHandlerTest.java
Show resolved
Hide resolved
…tty-12.1.x/delayedDispatch
# Conflicts: # jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java # jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java # jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/DelayedHandlerTest.java
|
@sbordet nudge? |
|
@sbordet nudge!!!! |
…tty-12.1.x/delayedDispatch
…tty-12.1.x/delayedDispatch # Conflicts: # jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HTTP2ServerConnection.java # jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java
…tty-12.1.x/delayedDispatch
gregw
dismissed stale reviews from sbordet and lachlan-roberts
waited too long for re-review
| if (!request.getConnectionMetaData().getHttpConfiguration().isDelayDispatchUntilContent()) | ||
| return null; | ||
| // are we configured to delay dispatch until content? | ||
| boolean delayDispatchUntilContent = request.getConnectionMetaData().getHttpConfiguration().isDelayDispatchUntilContent(); |
There was a problem hiding this comment.
Why is this a setting on HttpConfiguration and not the DelayedHandler?
Seems like this setting only has any effect if you add a DelayedHandler.
There was a problem hiding this comment.
My personal preference is for this HttpConfig setting to be deprecated.
| * parsed into {@link FormFields} or {@link MultiPartFormData.Parts} prior to handling. | ||
| * <p> | ||
| * This handler can allow a blocking application to run without blocking on input, as the content is asynchronously | ||
| * read before the application is called. |
There was a problem hiding this comment.
Maybe you should specify how to set the MultiPartConfig so it is found by this handler. And how to specify the maxFormKeys and maxFormContentSize.
Because these won't be automatically set just because they have a servlet MutliPartConfigElement.
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/DelayedHandler.java
Show resolved
Hide resolved
| { | ||
| // Add padding content to avoid compaction | ||
| padding = buffer.limit(); | ||
| buffer.position(0); |
There was a problem hiding this comment.
This is some magic. This line has to know what the next line does, but I guess it has to be this way. It is just that all the times will read this line and think "why again are we setting the position to 0?".
It is what it is -- but I had to rant a little 😺
| been asynchronously read. For all other content types, the delay is for up to one | ||
| input buffer of content. |
There was a problem hiding this comment.
I don't think this is correct? The implementation has a maxSize parameter that could be set to Integer.MAX_VALUE and read way more than 1 buffer.
| /** | ||
| * @return The minimum space available in a retained input buffer before allocating a new one. | ||
| */ | ||
| public int getMinInputBufferSpace() |
There was a problem hiding this comment.
Don't particularly like the name, but cannot come up with a better one.
I miss the fact that this is only for retained input buffers, so perhaps getRetainedInputBufferMinSpace()?
Please also add the @ManagedAttribute annotation to the getter.
| * be {@code true} and up to {@link HttpConfiguration#getInputBufferSize()} of data may be | ||
| * {@link RetainableByteBuffer#retain() retained}. Once read, the data is made available via the standard |
There was a problem hiding this comment.
Same comment as the module, but see also comments about the implementation.
I'm not that keen to specify "up to inputBufferSize of data", I'd like either the first chunk, or up to the value I have configured.
| }; | ||
| } | ||
|
|
||
| protected DelayedProcess newUntilContentDelayedProcess(Handler handler, Request request, Response response, Callback callback) | ||
| { | ||
| return new UntilContentDelayedProcess(handler, request, response, callback, -1); |
There was a problem hiding this comment.
The last parameter -1 is hardcoded, so applications have no way to specify a value without subclassing, or by specifying a property in the module.
I think it should be a long and be configurable in DelayedHandler as a setter.
I was thinking that the value 0 would mean "wait until the first chunk", as well as 1.
Larger values would cap the bytes in the chunk(s).
Then, -1 could have the meaning of "read it until the last chunk", like form and multipart do.
Default would be 0, i.e. until first chunk.
I would switch to a long because 2 GiB may not that much for specific use cases.
| Content.Chunk chunk = super.getRequest().read(); | ||
| if (chunk == null) | ||
| { | ||
| getRequest().demand(org.eclipse.jetty.util.thread.Invocable.from(InvocationType.NON_BLOCKING, this::onContentAvailable)); |
There was a problem hiding this comment.
There may be more invocations of this line than the only one time run() is called, so I would switch the 2: here call demand(this), and when it's time to call the next Handler use a lambda.
Can't this code be written with a Promise like form and multipart, and use the same AtomicInteger(2) technique instead of calling execute()?
| ensureRequestBuffer(); | ||
| // If there is sufficient space available, we can top up the buffer rather than allocate a new one | ||
| ByteBuffer backing = _requestBuffer.getByteBuffer(); | ||
| if (BufferUtil.space(backing) >= getHttpConfiguration().getMinInputBufferSpace()) |
There was a problem hiding this comment.
This check should test for minInputBufferSpace > 0 && space > minInputBufferSpace like HTTP/2 does.
| // Here we know that this.networkBuffer is not retained by | ||
| // application code: either it has been released, or it's a new one. | ||
| int filled = fill(getEndPoint(), networkBuffer.getByteBuffer()); | ||
| int filled = fill(getEndPoint(), networkBuffer.getByteBuffer(), compact); |
There was a problem hiding this comment.
In HTTP/1 we keep the buffer around if filled<=0, since it's retained anyway by the application.
Should not we do the same here?
| if (filled <= 0) | ||
| { | ||
| releaseRequestBuffer(); | ||
| // Keep the buffer if it is retained | ||
| if (filled < 0 || !_requestBuffer.isRetained()) |
There was a problem hiding this comment.
Don't I want to keep around the buffer only if there is enough space?
So here there should be another check against minInputBufferSpace.
| @@ -1724,4 +1729,97 @@ public boolean handle(Request request, Response response, Callback callback) thr | |||
| else | |||
| assertThat(response.get(HttpHeader.CONNECTION), is(expectedConnectionHeader)); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testRetainedChunks() throws Exception | |||
There was a problem hiding this comment.
This test should be moved to jetty-test-client-transport to test it with all transports (apart FCGI).
Fix #9051. Revert to delayed content handling in the Connections