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-12T10:34:15Z

@gregw I've been investigating #12776 which happens to be a user-reported bug that is related to the cancel semantic and it appears that 11.0.x, 12.0.x and 12.1.x are buggy.

Cancelling was implemented in 12.0.x with a semantic close to 11.0.x (remove the buffer from the pool) but the mechanism is buggy. Then it was decided to implement it correctly for 12.1.x (hence the changes around IteratingCallback.abort()) so removing the buffers from the pool wouldn't be needed but that was not complete as we figured out more work is needed, and the buffer removing logic was partially added.

So we now have:

buggy 10/11 branches with a known regression and potential fix to restore the buffer removal code (Jetty 10.0.x: avoid buffer use after repooling #12786)
a buggy 12.0.x branch which uses the buffer removal mechanism that seems to be racy
a buggy 12.1.x branch with a currently broken write cancelling mechanism

gregw · 2025-02-12T13:45:00Z

@lorban can you re-review this branch and tell me exactly what you think are the current "broken write cancelling mechanism"? I've forgotten what we discuss last time we hungout on this.

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

lorban · 2025-02-12T15:36:17Z

@gregw just to be sure: I was speaking about jetty-12.1.x and not this branch when I wrote "currently broken write cancelling mechanism".

Here is what can happen for H2 that I call broken:

App calls Response.write(false, buffer, callback), which calls ChannelResponse.write(false, buffer, callback), which calls HttpStream.send(), which creates a DataFrame (which is a wrapper for the buffer) then calls HTTP2Session.data(h2stream, dataFrame, callback) which creates a HTTP2Session.Entry (which is a wrapper for both DataFrame and Callback) then appends it into its HTTP2Flusher instance.
Iterating the HTTP2Flusher calls HTTP2Session.Entry.generate(RBB.Mutable), which eventually calls DataGenerator.generateFrame() which calls RetainableByteBuffer.DynamicCapacity.add() to keep a ref onto the HTTP2Session.Entry instance. Eventually, the RetainableByteBuffer.DynamicCapacity is written to the HTTP2Session's EndPoint.
While the write is in progress, a H2 reset arrives; HTTP2Stream.onReset() is called which eventually calls HttpChannel.onRemoteFailure(x), which calls ChannelResponse.lockedFailWrite(x).
lockedFailWrite() calls _writeCallback.fail() (_writeCallback being the callback passed to Response.write()); this signals that the buffer passed to Response.write() can be released.
There is now a buffer that is both back into the pool and in an endpoint's flusher, possibly given to an in-progress SocketChannel.write(ByteBuffer[]) call.

…tate 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-14T13:41:11Z

@gregw The H1 implementation has been reworked and now contains tests.

What this PR lacks before it can be considered complete is some tests for the H2 impl, 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 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 2 commits February 12, 2025 15:09

make HttpStream.cancelSend() return Runnable

a7b2014

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

FCGI stream cancel implementation

808470b

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

review H1 cancellation + make EndPoint.cancelWrite go to the failed s…

60a1fc1

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

lorban linked an issue Feb 13, 2025 that may be closed by this pull request

ByteBufferPool - all ongoing requests fail when single request is cancelled on HTTP/2 #12776

Open

lorban mentioned this pull request Feb 13, 2025

ByteBufferPool - all ongoing requests fail when single request is cancelled on HTTP/2 #12776

Open

lorban added 12 commits February 13, 2025 19:46

defer H3 to after #12742

87206d9

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

Fix H2

63ed8e2

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

minor H1 improvement

a2b610b

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

add todos

6e47a47

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

align FCGI with H1

4912d8f

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

replace state CANCELLED with state FAILED

5e67a6d

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

add H1 write cancellation test

628639b

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

add H1 write cancellation test

6e84081

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

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

ae8c796

…1.x/cancelWrite

revert test prototype

23034cf

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

rollback blank line

25297c8

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

change mock impl

9c20685

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

lorban self-requested a review February 14, 2025 13:34

lorban added 3 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]>

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 12, 2025

gregw commented Feb 12, 2025

lorban commented Feb 12, 2025

lorban commented Feb 14, 2025 •

edited

Loading

		@@ -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 12, 2025

gregw commented Feb 12, 2025

lorban commented Feb 12, 2025

lorban commented Feb 14, 2025 • edited Loading

lorban commented Feb 14, 2025 •

edited

Loading