Jetty 12 idletimeout #9905

gregw · 2023-06-13T07:12:49Z

Rework idle timeout mechanism, undisable tests

+ pass TimeoutException through all APIs + HttpConnection now passes on TimeoutException to HttpChannel.onFailure

…timeout

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

* Improved reset of the earliest timeout before iteration. * Removed check for getExpireNanoTime() == -1, since it's a valid value. * When onExpired(Expirable) returns false, the Expirable should arrange to move its timeout in the future. Signed-off-by: Simone Bordet <[email protected]>

…-idletimeout

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

….failed() in all cases to complete the wrapped callback. Signed-off-by: Simone Bordet <[email protected]>

…ll super.failed() in all cases to complete the wrapped callback." This reverts commit 5ac57c1.

gregw · 2023-06-13T07:18:59Z

@lorban @lachlan-roberts The key changes in this PR are:

lots of simple mechanical changes to pass TimeoutException instead of Throwable when it is known to be a timeout.
Request has been updated to have two types of listeners: for failures and for timeouts. Failures cannot be ignored, timeouts can be.
HttpChannelState is where the majority of the changes have been made and to invoke the various listeners.
The production of an error response now uses a second ChannelResponse instance. This is to avoid the race where the original handler is still writing to the response whilst an error handler is trying to write the error page for an asynchronously delivered error.

# Conflicts: # jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java # jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpChannel.java

lorban

There are quite a few TODOs in the tests, but we can tackle them after the 12.0.0 release.

So LGTM.

lorban · 2023-06-13T10:41:57Z

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

@@ -1202,6 +1203,23 @@ public void call(Invocable.Callable callable, Request request) throws Exception
            }
        }

+        public <T> boolean test(Predicate<T> predicate, T t, Request request)


This looks unused. Should we keep it nevertheless?

@lorban It has been used before, but now that we don't have listening predicates..... oh but wait yes we do. This should be used by the context addIdleTimeoutListener wrapper that is not implemented!!!! stand by.....

@lorban I've added the context wrapper so idle timeouts are callback in the context. I've also added a test to check that. Please re-review... at least that last commit.

lorban · 2023-06-13T10:44:11Z

jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ConnectorTimeoutTest.java

+
+            // TODO what do we do about the failing write?
+            //      should timeout errors be non persistent?
+            Callback ocb = callback;


Should we keep this wrapping?

long term no, but whilst we still have work to do....

lorban · 2023-06-13T10:48:35Z

jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ServerTest.java

+    }
+
+    @Test
+    public void testIdleTimeoutNoListenerHttpConfigurationOnly() throws Exception


I fail to see what this test asserts that the above one (testIdleTimeoutNoListener()) does not.

This one has the idle timeout configured in HttpConfiguration only, so it is testing that it is correctly applied by HttpChannel. I.e the idle timeout between requests is 10x the idle timeout during requests.

(probably could have been parameterised... will do when I next touch this test.)

...t/src/main/java/org/eclipse/jetty/fcgi/client/transport/internal/HttpConnectionOverFCGI.java

jetty-core/jetty-io/src/test/java/org/eclipse/jetty/io/ByteArrayEndPointTest.java

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

gregw and others added 26 commits June 6, 2023 18:01

IdleTimeout review

95a1af1

+ pass TimeoutException through all APIs + HttpConnection now passes on TimeoutException to HttpChannel.onFailure

Simpler idle test

fd113c8

More ServerTests for idletimeout

fe989b7

More ServerTests for idletimeout

18cd25e

Merge remote-tracking branch 'origin/jetty-12.0.x' into jetty-12-idle…

a792a76

…timeout

Fixed FCGI

f2a5845

Merge remote-tracking branch 'origin/jetty-12.0.x' into jetty-12-idle…

d4f3271

…timeout

Recreated a ServerTimeoutsTest for multiple transports

eb63a1b

more robust tests

a8544bc

WIP.

185f3bd

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

merged work from @sbordet and @gregw

b75bbb5

Merge branch 'fix/jetty-12-cyclictimeouts-improvements' into jetty-12…

9733888

…-idletimeout

Fixed demand with idle timeout true

538bd5e

WIP idleTimeout

6a36479

WIP idleTimeout

28d63c2

WIP idleTimeout

c748b54

WIP idleTimeout

c232547

fix keystore to please BoringSSL + use correct temp path

450c03a

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

WIP idleTimeout

f745d0a

WIP idleTimeout

1a0ee0b

WIP idleTimeout

0dbbdf5

Fixed ErrorResponseAndCallback succeeded() and failed() to call super…

5ac57c1

….failed() in all cases to complete the wrapped callback. Signed-off-by: Simone Bordet <[email protected]>

Revert "Fixed ErrorResponseAndCallback succeeded() and failed() to ca…

cf39788

…ll super.failed() in all cases to complete the wrapped callback." This reverts commit 5ac57c1.

WIP idleTimeout

2ede51a

WIP idleTimeout

d014214

gregw requested review from lorban and sbordet June 13, 2023 07:12

gregw marked this pull request as ready for review June 13, 2023 07:13

gregw requested a review from lachlan-roberts June 13, 2023 07:13

Merge branch 'jetty-12.0.x' into jetty-12-idletimeout

96a24c2

# Conflicts: # jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java # jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpChannel.java

lorban previously approved these changes Jun 13, 2023

View reviewed changes

Added context wrapper for idle timeout listener

d4257e9

gregw dismissed lorban’s stale review via d4257e9 June 13, 2023 13:22

gregw requested a review from lorban June 13, 2023 13:23

lorban previously approved these changes Jun 13, 2023

View reviewed changes

lachlan-roberts requested changes Jun 13, 2023

View reviewed changes

updates from review

3aa70ba

gregw dismissed lorban’s stale review via 3aa70ba June 13, 2023 14:01

gregw requested review from lorban and lachlan-roberts June 13, 2023 14:02

lorban approved these changes Jun 13, 2023

View reviewed changes

lachlan-roberts approved these changes Jun 14, 2023

View reviewed changes

gregw merged commit 963d331 into jetty-12.0.x Jun 14, 2023

gregw deleted the jetty-12-idletimeout branch June 14, 2023 07:57

This was referenced Jun 14, 2023

Idle timeout is ignored if callback is not completed #9169

Closed

Fixes and extra testing for EE9 ContextHandler class loading #9878

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jetty 12 idletimeout #9905

Jetty 12 idletimeout #9905

gregw commented Jun 13, 2023

gregw commented Jun 13, 2023

lorban left a comment

lorban Jun 13, 2023

gregw Jun 13, 2023

gregw Jun 13, 2023

lorban Jun 13, 2023

gregw Jun 13, 2023

lorban Jun 13, 2023

gregw Jun 13, 2023

gregw Jun 13, 2023

Jetty 12 idletimeout #9905

Jetty 12 idletimeout #9905

Conversation

gregw commented Jun 13, 2023

gregw commented Jun 13, 2023

lorban left a 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