Issue #10513

lachlan-roberts · 2023-09-20T01:30:02Z

add Http2MultiPartServletTest to replicate the issue
proposed fix inside HTTP2Session.StreamData.

PR is not complete yet but worth getting some early reviews.

todo:

more improvements on the testing, also test HTTP/3 etc...
investigate why chunks aren't released if the part is being written to a file
form configuration from the ServletContextHandler is overriding configuration from the MultipartConfigElement

Signed-off-by: Lachlan Roberts <[email protected]>

lorban · 2023-09-20T08:40:55Z

I saw two failing tests in a PR build because of spurious leaks. Could you please disable leak tracking for them? You just have to annotate:

org.eclipse.jetty.test.client.transport.HttpClientStreamTest.testInputStreamContentProviderThrowingWhileReading with @Tag("DisableLeakTracking:server:H3")

and

org.eclipse.jetty.test.client.transport.HttpClientTimeoutTest.testVeryShortTimeout with

@Tag("DisableLeakTracking:client:HTTP")
@Tag("DisableLeakTracking:client:HTTPS")
@Tag("DisableLeakTracking:client:UNIX_DOMAIN")

lorban · 2023-09-20T08:42:34Z

...-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java

            }
-            return result;
+
+            // If all the data has been consumes and at least one release call is made, that indicates that the content


Typo: If all the data has been consumed

lorban · 2023-09-20T08:45:12Z

...tty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/Http2MultiPartServletTest.java

+    private void start(HttpServlet servlet, MultipartConfigElement config) throws Exception
+    {
+        config = config == null ? new MultipartConfigElement(tmpDirString, MAX_FILE_SIZE, -1, 0) : config;
+        server = new Server(null, null, null);


You should use a leak-tracking buffer pool. See HttpClientProxyProtocolTest as an example of how to use ArrayByteBufferPool.Tracking.

The other test already uses that and I'm deleting this test

lorban · 2023-09-20T08:49:05Z

...-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java

+            // Note that a release with a non-empty buffer, may indicate the release of just a part of the buffer and thus
+            // the data has not been consumed. An optimization might be to consume the data up to the current position on each
+            // release.
+            if (BufferUtil.isEmpty(data.frame().getByteBuffer()) && consumed.compareAndSet(false, true))


I'm not so sure about this one. How about the following oddity?

Buffer duplicate = buffer.slice(); buffer.get(...); // read everything duplicate.get(...); // try to read again

Wouldn't that be a nasty surprise?

After discussing this issue with @sbordet, we came to an agreement that the right place to open the flow control window is in HTTP2Stream.readData(), i.e.: right before handing the buffer to the application code, to decouple the window control from the re-pooling of the buffer. This would make the behavior in line with how H1 works, as the TCP window is controlled by when you read from the socket.

@lorban I don't understand the surprise? If a make a duplicate, then of course I can read the data again from the buffer.

@sbordet @lorban Might gut reaction to the idea of opening the window in readData is NNNNNNNNOOOOOOOoooooo.... but I can't think of a rational reason why not.
That would make reading about consumption and retain/release about buffering. Good separation of concerns.

Let me try to implement....

sbordet · 2023-09-20T09:06:38Z

...t-transports/src/test/java/org/eclipse/jetty/test/client/transport/HttpClientStreamTest.java

+            @Override
+            public boolean handle(Request request, org.eclipse.jetty.server.Response response, Callback callback)
+            {
+                Runnable retainer = new Runnable()


You may find o.e.j.util.CompletableTask better to use instead of bare Runnable.

sbordet · 2023-09-20T09:07:06Z

...t-transports/src/test/java/org/eclipse/jetty/test/client/transport/HttpClientStreamTest.java

+        ByteBufferRequestContent content = new ByteBufferRequestContent(ByteBuffer.wrap(data));
+        client.newRequest(newURI(transport))
+            .body(content)
+            .send(new BufferingResponseListener(data.length)


You may use CompletableResponseListener here, if it's easier.

sbordet · 2023-09-20T09:07:40Z

...t-transports/src/test/java/org/eclipse/jetty/test/client/transport/HttpClientStreamTest.java

+                            if (Content.Chunk.isFailure(chunk))
+                            {
+                                callback.failed(chunk.getFailure());
+                                return;
+                            }


I'd move this just after the null check, as per usual idiom.

sbordet · 2023-09-20T09:08:08Z

...t-transports/src/test/java/org/eclipse/jetty/test/client/transport/HttpClientStreamTest.java

+                            if (chunk.hasRemaining())
+                            {
+                                ByteBuffer byteBuffer = chunk.getByteBuffer();
+                                if (chunk.canRetain())


It's a test, surely we can retain?

I don't know? Do all the transports provide retainable chunks?

sbordet · 2023-09-20T09:10:09Z

...tty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/Http2MultiPartServletTest.java

+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.equalTo;
+
+public class Http2MultiPartServletTest


Move this to module jetty-ee10-test-client-transport, so that you don't pollute ee10-servlet with http2 deps.

I think we can delete this test now, as we have a simpler reproducer that works for all transports in HttpClientStreamTest

Signed-off-by: Simone Bordet <[email protected]>

Signed-off-by: Ludovic Orban <[email protected]>

Signed-off-by: Simone Bordet <[email protected]>

Signed-off-by: Ludovic Orban <[email protected]>

...t-transports/src/test/java/org/eclipse/jetty/test/client/transport/HttpClientStreamTest.java

…ges. Signed-off-by: Simone Bordet <[email protected]>

Signed-off-by: Simone Bordet <[email protected]>

lorban

LGTM overall, but the tests could be slightly simplified.

...etty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/DataDemandTest.java

...2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/FlowControlStrategyTest.java

...etty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/DataDemandTest.java

...2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/FlowControlStrategyTest.java

Signed-off-by: Simone Bordet <[email protected]>

…onsumesDataOfInFlightStream()`, that became flaky after #10554. Signed-off-by: Simone Bordet <[email protected]>

…t became flaky after #10554. Signed-off-by: Simone Bordet <[email protected]>

Signed-off-by: Simone Bordet <[email protected]>

Signed-off-by: Ludovic Orban <[email protected]>

* Fixed more tests that became flaky after #10554. Signed-off-by: Ludovic Orban <[email protected]>

Issue #10513 - fix multipart lockup with HTTP/2

ddd6b64

Signed-off-by: Lachlan Roberts <[email protected]>

lachlan-roberts requested review from gregw and sbordet September 20, 2023 01:30

lachlan-roberts self-assigned this Sep 20, 2023

lachlan-roberts linked an issue Sep 20, 2023 that may be closed by this pull request

Lockup processing POST request body with Jetty 12.0.1 using http/2 #10513

Open

gregw added 2 commits September 20, 2023 16:20

Updated comments with more detail

7682591

Better structure

ef4ef1e

gregw removed their request for review September 20, 2023 06:26

gregw self-assigned this Sep 20, 2023

gregw requested a review from lorban September 20, 2023 06:26

lorban requested changes Sep 20, 2023

View reviewed changes

Better test without multipart

82f5f65

sbordet requested changes Sep 20, 2023

View reviewed changes

gregw and others added 7 commits September 20, 2023 21:26

Updates from review

3a7cc0a

Updates from review

9d59e41

Code cleanups.

705f53b

Signed-off-by: Simone Bordet <[email protected]>

reduce data size for H3 and add verbosity

68bb382

Signed-off-by: Ludovic Orban <[email protected]>

Removed unnecessary dependencies.

00510b8

Signed-off-by: Simone Bordet <[email protected]>

disable leak tracking for H1

7799f62

Signed-off-by: Ludovic Orban <[email protected]>

Updates from review

68de6af

sbordet approved these changes Sep 26, 2023

View reviewed changes

lorban reviewed Sep 26, 2023

View reviewed changes

...t-transports/src/test/java/org/eclipse/jetty/test/client/transport/HttpClientStreamTest.java Show resolved Hide resolved

gregw and others added 4 commits September 27, 2023 09:36

Updates from review

b288b14

Updated FlowControlStrategyTest after Stream.readData() semantic chan…

f400e4c

…ges. Signed-off-by: Simone Bordet <[email protected]>

Updated documentation.

8d7523f

Signed-off-by: Simone Bordet <[email protected]>

Merged branch 'jetty-12.0.x' into 'jetty-12.0.x-10513-multipart-http2'.

28c9fe9

Signed-off-by: Simone Bordet <[email protected]>

sbordet requested review from gregw, sbordet and lorban September 27, 2023 17:55

sbordet added 2 commits September 27, 2023 21:40

Updated DataDemandTest after Stream.readData() semantic changes.

66067fd

Signed-off-by: Simone Bordet <[email protected]>

Disabled test for FCGI.

26e2457

Signed-off-by: Simone Bordet <[email protected]>

lorban requested changes Sep 28, 2023

View reviewed changes

Updates after review.

7b689f6

Signed-off-by: Simone Bordet <[email protected]>

sbordet requested a review from lorban September 28, 2023 08:37

lorban approved these changes Sep 28, 2023

View reviewed changes

sbordet merged commit aa880cf into jetty-12.0.x Sep 28, 2023

sbordet deleted the jetty-12.0.x-10513-multipart-http2 branch September 28, 2023 09:47

sbordet mentioned this pull request Sep 28, 2023

Lockup processing POST request body with Jetty 12.0.1 using http/2 #10513

Open

sbordet added a commit that referenced this pull request Oct 5, 2023

Fixed flaky test `GoAwayTest.testServerGoAwayWithStalledStreamServerC…

d833812

…onsumesDataOfInFlightStream()`, that became flaky after #10554. Signed-off-by: Simone Bordet <[email protected]>

sbordet added a commit that referenced this pull request Oct 5, 2023

Fixed flaky test StreamResetTest.testResetAfterBlockingWrite(), tha…

a4b55e6

…t became flaky after #10554. Signed-off-by: Simone Bordet <[email protected]>

sbordet added a commit that referenced this pull request Oct 5, 2023

Fixed test FlowControlStrategyTest after the changes of #10554.

ff57574

Signed-off-by: Simone Bordet <[email protected]>

lorban added a commit that referenced this pull request Oct 12, 2023

Fixed more tests that became flaky after #10554.

dac729e

Signed-off-by: Ludovic Orban <[email protected]>

lorban mentioned this pull request Oct 12, 2023

Fix flaky leaky h2 tests #10728

Merged

sbordet pushed a commit that referenced this pull request Oct 13, 2023

Fix flaky leaky h2 tests (#10728)

d6c900a

* Fixed more tests that became flaky after #10554. Signed-off-by: Ludovic Orban <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #10513 - fix multipart lockup with HTTP/2 #10554

Issue #10513 - fix multipart lockup with HTTP/2 #10554

lachlan-roberts commented Sep 20, 2023

lorban commented Sep 20, 2023

lorban Sep 20, 2023

lorban Sep 20, 2023

gregw Sep 20, 2023

lorban Sep 20, 2023

lorban Sep 20, 2023

gregw Sep 20, 2023

sbordet Sep 20, 2023

sbordet Sep 20, 2023

sbordet Sep 20, 2023

sbordet Sep 20, 2023

gregw Sep 20, 2023

sbordet Sep 20, 2023

gregw Sep 20, 2023

lorban left a comment

Issue #10513 - fix multipart lockup with HTTP/2 #10554

Issue #10513 - fix multipart lockup with HTTP/2 #10554

Conversation

lachlan-roberts commented Sep 20, 2023

Issue #10513

lorban commented Sep 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorban left a comment

Choose a reason for hiding this comment