Cancel write semantic #12727

gregw · 2025-01-22T08:09:05Z

Added a cancel write/send semantic so that buffers for failed writes need not be removed from the pool

Provide a write cancel mechanism so that removing pooled buffers can be avoided.

gregw · 2025-01-22T08:25:51Z

@sbordet can you implement the cancel semantic for http2. @lorban ditto for http3

Provide a write cancel mechanism so that removing pooled buffers can be avoided.

…1.x/cancelWrite

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

gregw · 2025-01-28T15:52:39Z

...y-core/jetty-http3/jetty-http3-common/src/main/java/org/eclipse/jetty/http3/HTTP3Stream.java

+    public Callback cancel(Throwable cause, Callback callback)
+    {
+        Callback nested = new Callback.Nested(callback)
+        {
+            @Override
+            public void succeeded()
+            {
+                super.failed(cause);
+            }
+
+            @Override
+            public void failed(Throwable x)
+            {
+                ExceptionUtil.addSuppressedIfNotAssociated(cause, x);
+                super.failed(cause);
+            }
+        };
+        reset(HTTP3ErrorCode.REQUEST_CANCELLED_ERROR.code(), cause);
+        return nested;
+    }


@lorban that looks simple enough. Same as h2. Any idea how to test this?

This is the big question: the implementation looks fairly easy but I've been scratching my head trying to find a way to reliably test that.

Same comment as HTTP/2.

gregw · 2025-01-29T15:06:31Z

@sbordet @lorban nudge for actual review

sbordet

I am dubious about the H2/h3 implementation; we should talk about it.

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpStream.java

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/WriteFlusher.java

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/WriteFlusher.java

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

lorban · 2025-01-31T12:31:35Z

...y-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/frames/DisconnectFrame.java

@@ -13,6 +13,7 @@

 package org.eclipse.jetty.http2.frames;

+@Deprecated (forRemoval = true, since = "12.1.0")


Why deprecating this?

because it is not used

...ty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/internal/HTTP2Flusher.java

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/EndPoint.java

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpStream.java

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/WriteFlusher.java

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java

sbordet · 2025-01-31T15:54:00Z

@gregw another issue is the use of CountingCallback: it has the semantic to count for successes, but it fails on the first failure.

So it has not the right semantic for the usage that was done in H2 and H3.

gregw · 2025-02-04T06:42:12Z

@gregw another issue is the use of CountingCallback: it has the semantic to count for successes, but it fails on the first failure.

So it has not the right semantic for the usage that was done in H2 and H3.

@sbordet See new static List<Callback> from(Callback callback, Throwable cause, int count) method that replaces CountingCallback

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

lorban

I believe the mechanism is sane, but I can't wrap my head around the newly introduced API and how this should all work.

I've attempted to write a test for this functionality (I've pushed it to this PR) and that left me puzzled. We should probably discuss it.

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/WriteFlusher.java

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ErrorHandler.java

gregw · 2025-02-12T09:32:08Z

@sbordet @lorban have you looked at this recently? Any updates for h2 or h3?

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

lorban · 2025-02-14T13:41:11Z

@gregw The H1 and H2 implementations have been reworked and are now tested.

What this PR lacks before it can be considered complete is handling a set of TODOs, javadoc, naming and minor potential improvements.

The H3 implementation has been deferred to after #12742 is merged, as that other PR changes too drastically the QUIC/H3 interactions so any work on the current impl would need to be completely re-done.

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

gregw · 2025-02-18T18:33:40Z

@lorban can you re-review this PR and merge it?

lorban · 2025-02-19T10:34:20Z

@gregw Not yet, as I would like a firm confirmation that this PR solves the problem once and for all.

The #12776 issue is exactly about this bug, but not only: yesterday we discovered together with @sbordet that we have an extra bug (#12805) that still makes that user's use-case fail despite the fix in this PR.

I would like to get a confirmation from this user that 12.1 is working before merging this PR as he's been very willing to help, responsive and came up with a reliable reproducer.

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

… end already sent one Signed-off-by: Ludovic Orban <[email protected]>

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

…1.x/cancelWrite

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

lorban · 2025-02-21T14:33:43Z

The H3 implementation will come after #12742 gets merged.

lorban · 2025-02-21T15:24:42Z

@gregw @sbordet this PR is ready for its final review as I consider it ready to be merged

sbordet · 2025-02-21T17:11:36Z

...-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java

+            if (LOG.isDebugEnabled())
+                LOG.debug("cancelSend reset callback complete");
+            appCallback.failed(cause);
+        }));


_stream.reset() may still return early.

How about introducing a HTTP2Session.flush(Callback) that queues a "fake" FlushEntry into the HTTP2FLusher whose only scope is to be flushed and notify the callback when done?

I'll do that, and I'll also write a test similar to testResetAfterTCPCongestedWrite() that waits for idle timeout instead of resolving the TCP congestion.

gregw added 2 commits January 22, 2025 17:41

first attempt

ffd68fc

Do not remove pooled buffers on write failure

ba00bcf

Provide a write cancel mechanism so that removing pooled buffers can be avoided.

gregw requested review from sbordet and lorban January 22, 2025 08:09

gregw and others added 6 commits January 22, 2025 12:48

Do not remove pooled buffers on write failure

04d420c

Provide a write cancel mechanism so that removing pooled buffers can be avoided.

Merge remote-tracking branch 'origin/jetty-12.1.x' into fix/jetty-12.…

fcef2e5

…1.x/cancelWrite

HTTP2 WIP

75dfeb3

HTTP2 WIP

0b16d61

HTTP2 use reset to cancel

737e9e5

add h3 support

05a0fec

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

gregw commented Jan 28, 2025

View reviewed changes

sbordet requested changes Jan 31, 2025

View reviewed changes

lorban requested changes Jan 31, 2025

View reviewed changes

updates from review

0becca7

gregw requested review from lorban and sbordet January 31, 2025 15:53

gregw added 2 commits January 31, 2025 17:15

updates from review

f538286

removed CountingCallback usage

cf82fcc

test tentative

50cc8cb

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

lorban requested changes Feb 4, 2025

View reviewed changes

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/ErrorHandler.java Show resolved Hide resolved

updates from review

613a40c

gregw mentioned this pull request Feb 12, 2025

Jetty 10.0.x: avoid buffer use after repooling #12786

Merged

gregw and others added 2 commits February 12, 2025 10:51

Merge branch 'jetty-12.1.x' into fix/jetty-12.1.x/cancelWrite

338754d

make HttpStream.cancelSend() non-default

f02a8dd

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

lorban added 8 commits February 14, 2025 14:42

fix incorrect fall through from previous branch of the switch statement

4a87adc

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

add WriteFlusher.cancelWrite() test and javadoc

aa19b92

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

fix checkstyle

7ef84b6

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

add H2 test

cc1e13d

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

improve tests

48c90f4

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

improve tests

55afda1

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

improve tests

282407c

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

fix javadoc

60c100e

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

lorban self-assigned this Feb 19, 2025

lorban added Bug For general bugs on Jetty side High Priority labels Feb 19, 2025

lorban added 2 commits February 21, 2025 09:06

fix log

b4407d5

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

fix Http2Stream.reset() failing to send a reset frame when the remote…

81927fe

… end already sent one Signed-off-by: Ludovic Orban <[email protected]>

lorban force-pushed the fix/jetty-12.1.x/cancelWrite branch from 8d5dbd0 to 81927fe Compare February 21, 2025 08:09

lorban added 2 commits February 21, 2025 09:16

improve javadoc

61a7118

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

improve javadoc

4849197

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

lorban mentioned this pull request Feb 21, 2025

Request.addCompletionListener() may run the listener before the request is fully complete #12805

Closed

lorban added 7 commits February 21, 2025 10:12

fix test

219dec8

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

fix H2 stream endpoint

cc5596b

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

remove all traces of releaseAndRemove

f68ed8b

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

Merge remote-tracking branch 'origin/jetty-12.1.x' into fix/jetty-12.…

b53966d

…1.x/cancelWrite

Merge remote-tracking branch 'origin/jetty-12.1.x' into fix/jetty-12.…

3424438

…1.x/cancelWrite

Merge remote-tracking branch 'origin/jetty-12.1.x' into fix/jetty-12.…

6f744df

…1.x/cancelWrite

remove todos

4066804

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

sbordet requested changes Feb 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cancel write semantic #12727

Cancel write semantic #12727

gregw commented Jan 22, 2025

gregw commented Jan 22, 2025

gregw Jan 28, 2025

lorban Jan 28, 2025

sbordet Jan 31, 2025

gregw commented Jan 29, 2025

sbordet left a comment

lorban Jan 31, 2025

gregw Jan 31, 2025

sbordet commented Jan 31, 2025

gregw commented Feb 4, 2025

lorban left a comment

gregw commented Feb 12, 2025

lorban commented Feb 14, 2025 •

edited

Loading

gregw commented Feb 18, 2025

lorban commented Feb 19, 2025

lorban commented Feb 21, 2025

lorban commented Feb 21, 2025

sbordet Feb 21, 2025

lorban Feb 21, 2025

		@@ -13,6 +13,7 @@

		package org.eclipse.jetty.http2.frames;

		@Deprecated (forRemoval = true, since = "12.1.0")

Cancel write semantic #12727

Are you sure you want to change the base?

Cancel write semantic #12727

Conversation

gregw commented Jan 22, 2025

gregw commented Jan 22, 2025

gregw Jan 28, 2025

Choose a reason for hiding this comment

lorban Jan 28, 2025

Choose a reason for hiding this comment

sbordet Jan 31, 2025

Choose a reason for hiding this comment

gregw commented Jan 29, 2025

sbordet left a comment

Choose a reason for hiding this comment

lorban Jan 31, 2025

Choose a reason for hiding this comment

gregw Jan 31, 2025

Choose a reason for hiding this comment

sbordet commented Jan 31, 2025

gregw commented Feb 4, 2025

lorban left a comment

Choose a reason for hiding this comment

gregw commented Feb 12, 2025

lorban commented Feb 14, 2025 • edited Loading

gregw commented Feb 18, 2025

lorban commented Feb 19, 2025

lorban commented Feb 21, 2025

lorban commented Feb 21, 2025

sbordet Feb 21, 2025

Choose a reason for hiding this comment

lorban Feb 21, 2025

Choose a reason for hiding this comment

lorban commented Feb 14, 2025 •

edited

Loading