Jetty 12 handler as boolean processor #9035

gregw · 2022-12-11T01:21:54Z

Another alternative Handler architecture inspired by #8978 for #8929

This PR removed the modal nature of Request, Response and Callback, so that they need not be accepted (via API or returning a Processor) before they are fully active. Not only does this avoid a lot of locking on many methods and on the state change, but it also allows for a boolean return from process to no longer be a race condition.

If a handler.process(request,response,callback) returns true, then the callback will be called and the caller must no longer act on the request/response/callback. If false is returned, then the handler will never act on the request/response/callback so the caller must either handle the request itself (e.g. 404) or it is free to try another handler.
If an exception is thrown, then the caller cannot know if the callback will be called and must assume not and race with any lingering async process to write an error and complete the callback.

In #8978, I tried having a separate Handler.Conditional type that would have a method on it to accept requests and only then call process which had to handle the request. This is probably workable but had several cons:

difficult to pass state/workings/wrappers from the conditional test to the process method, so some work is done twice or we need wrappers just to carry extra state.
It was difficult to wrap conditional handlers with non conditional handlers.
callers had to do instanceof test of handlers before knowing how to call them.

So this branch is simpler, as essentially all handlers are conditional.

VERSION.txt

All Handlers are Processors, which now return a boolean to indicate the request has been accepted. The request/response/callback are no longer modal, so there is no race with the boolean return.

gregw · 2022-12-11T21:49:10Z

documentation/jetty-asciidoctor-extensions/pom.xml

-  <properties>
-    <bundle-symbolic-name>${project.groupId}.asciidoctor.extensions</bundle-symbolic-name>
-  </properties>
-


Hmm I've still missed some changes in HEAD?

Fixes from bad squash

…ler-as-boolean-processor

janbartel

I really like the way this is going. The code is much simpler. It has to also be faster.

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

Minor renames to reduce differences

gregw · 2022-12-12T05:35:10Z

jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/ContextHandlerTest.java

-            "Core enter http://0.0.0.0/",
-            "EE9 enter /",
-            "EE9 exit /",
-            "Core exit http://0.0.0.0/",
-            // Enter again for process(request, response, callback)


This one makes me smile :)

Minor cleanups to reduce differences

Fixed race in test

Fixed missed merge

Non generic DelayedHandler

restored comment

inlined the Handler.Processor class

gregw · 2022-12-13T03:02:48Z

@sbordet @lorban I've been looking at redoing this branch from HEAD, and I really don't think that is the right way to go. There is a lot of work and consideration in this branch that will take a huge effort to redo from scratch. I know there is some risk of some changes being lost in this branch, but I honestly think that is less if we review this branch and check for them, than if we redo the changes to the many hundreds of handlers. Whilst much of it can be done with tools, ultimately you need to visit most of the handlers to check them. I've visited many hundreds of handlers and believe I have them mostly right. Redoing all that work will not remove the need for a detailed review to recheck them all.

I don't think there is any significant kruft from previous branches left, and if there is it needs to be reviewed, identified and labelled as kruft anyway (rather than a change to be remade). Again, I don't think it is less work to redo the branch rather than review this one.

…te/handler/Rule.java Co-authored-by: Simone Bordet <simone.bordet@gmail.com>

…b.com/eclipse/jetty.project into jetty-12-handler-as-boolean-processor

Avoid iterations if only ServletPathSpec instances Avoid tests for empty mappings. Better reset implementation

Improve suffix matching

Improve exact matching

…ler-as-boolean-processor

sbordet

LGTM. I have a few nits, but I'll fix them myself.

sbordet · 2022-12-19T09:01:58Z

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

+     * which is appropriate for wall clock time.</p>
+     * @return The timestamp that the request was received/created in nanoseconds, relative to {@link System#nanoTime()}.
+     */
+    default long getNanoTimeStamp()


Rename to getNanoTime().

sbordet · 2022-12-19T09:03:01Z

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

+     */
+    default long getNanoTimeStamp()
+    {
+        return NanoTime.now() - TimeUnit.MILLISECONDS.toNanos(System.currentTimeMillis() - getTimeStamp());


I don't think this implementation is correct, since currentTimeMillis() can drift.
Must use another field to store the creation/received nanoTime.

@sbordet we do. It is in the HttpStream. This is just used for test code.

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

sbordet · 2022-12-19T09:34:32Z

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

+            if (stream != null)
+                return stream.getNanoTimeStamp();
+
+            return NanoTime.now() - TimeUnit.MILLISECONDS.toNanos(System.currentTimeMillis() - getTimeStamp());


Should call super instead?
As we don't want to duplicate the (wrong) logic here.

Does super call a default implementation? I don't think so.

Renamed HttpStream.getNanoTimeStamp() to getNanoTime(). Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

…sor'. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

lorban

LGTM

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

gregw requested review from sbordet, joakime and lachlan-roberts and removed request for sbordet and joakime December 11, 2022 01:21

gregw added the Jetty 12 label Dec 11, 2022

gregw requested review from joakime, lorban, janbartel and olamy December 11, 2022 01:22

gregw force-pushed the jetty-12-handler-as-boolean-processor branch from a35b2e9 to c6d691d Compare December 11, 2022 01:26

gregw marked this pull request as ready for review December 11, 2022 05:43

gregw commented Dec 11, 2022

View reviewed changes

VERSION.txt Outdated Show resolved Hide resolved

Alternative Handler architecture

4b4a46a

All Handlers are Processors, which now return a boolean to indicate the request has been accepted. The request/response/callback are no longer modal, so there is no race with the boolean return.

gregw force-pushed the jetty-12-handler-as-boolean-processor branch from 0ed443b to 4b4a46a Compare December 11, 2022 21:47

gregw commented Dec 11, 2022

View reviewed changes

gregw added 2 commits December 12, 2022 15:35

Alternative Handler architecture

b21c308

Fixes from bad squash

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

8869899

…ler-as-boolean-processor

janbartel approved these changes Dec 12, 2022

View reviewed changes

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

Alternative Handler architecture

9a272b3

Minor renames to reduce differences

gregw commented Dec 12, 2022

View reviewed changes

gregw added 7 commits December 12, 2022 16:36

Alternative Handler architecture

6bb840b

Minor cleanups to reduce differences

Alternative Handler architecture

dcf9038

Fixed race in test

Alternative Handler architecture

3338015

Fixed missed merge

Alternative Handler architecture

6697b59

Non generic DelayedHandler

Alternative Handler architecture

2f8950d

restored comment

Merge branch 'jetty-12.0.x' into jetty-12-handler-as-boolean-processor

63cf4d3

Alternative Handler architecture

537edc8

inlined the Handler.Processor class

gregw and others added 9 commits December 15, 2022 11:53

updates from @lorban review

2561565

Update jetty-core/jetty-rewrite/src/main/java/org/eclipse/jetty/rewri…

63ed0ae

…te/handler/Rule.java Co-authored-by: Simone Bordet <simone.bordet@gmail.com>

updates from @sbordet review

04ff98e

Merge branch 'jetty-12-handler-as-boolean-processor' of https://githu…

ba9dd57

…b.com/eclipse/jetty.project into jetty-12-handler-as-boolean-processor

updates from @sbordet review

4674291

updates from @sbordet review

3dd9f0e

updates from @sbordet review

a43f644

updates from @sbordet review

09c3449

updates from @sbordet review

7cab823

gregw requested review from sbordet, janbartel and lorban December 15, 2022 12:38

gregw added 5 commits December 16, 2022 09:21

Optimize PathMappings

80070ab

Avoid iterations if only ServletPathSpec instances Avoid tests for empty mappings. Better reset implementation

Optimize PathMappings

a49f053

Avoid iterations if only ServletPathSpec instances Avoid tests for empty mappings. Better reset implementation

Optimize PathMappings

a13387f

Improve suffix matching

Optimize PathMappings

7328a99

Improve exact matching

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

5d31900

…ler-as-boolean-processor

sbordet approved these changes Dec 19, 2022

View reviewed changes

sbordet added 2 commits December 19, 2022 11:10

Cleaned up Request.getNanoTime().

b5c17fd

Renamed HttpStream.getNanoTimeStamp() to getNanoTime(). Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Merged branch 'jetty-12.0.x' into 'jetty-12-handler-as-boolean-proces…

f1ad068

…sor'. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

lorban approved these changes Dec 19, 2022

View reviewed changes

sbordet added 2 commits December 19, 2022 11:41

Avoid using an extra field to store nanoTime.

186747e

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Now throw from getNanoTime() if there is no stream.

77fa741

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

sbordet merged commit c18e790 into jetty-12.0.x Dec 19, 2022

sbordet deleted the jetty-12-handler-as-boolean-processor branch December 19, 2022 15:02

gregw restored the jetty-12-handler-as-boolean-processor branch December 19, 2022 20:20

gregw deleted the jetty-12-handler-as-boolean-processor branch December 19, 2022 20:21

olamy mentioned this pull request Dec 26, 2022

keep matching exception to found errorpage #9090

Merged

gregw mentioned this pull request Jan 18, 2023

PathMappings efficiency #7844

Closed

joakime mentioned this pull request Jan 25, 2023

Jetty-12 Callback wrapping. #7728

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jetty 12 handler as boolean processor #9035

Jetty 12 handler as boolean processor #9035

gregw commented Dec 11, 2022 •

edited by sbordet

Loading

gregw Dec 11, 2022

janbartel left a comment

gregw Dec 12, 2022

gregw commented Dec 13, 2022

sbordet left a comment

sbordet Dec 19, 2022

sbordet Dec 19, 2022

gregw Dec 19, 2022

sbordet Dec 19, 2022

gregw Dec 19, 2022

lorban left a comment

Jetty 12 handler as boolean processor #9035

Jetty 12 handler as boolean processor #9035

Conversation

gregw commented Dec 11, 2022 • edited by sbordet Loading

gregw Dec 11, 2022

Choose a reason for hiding this comment

janbartel left a comment

Choose a reason for hiding this comment

gregw Dec 12, 2022

Choose a reason for hiding this comment

gregw commented Dec 13, 2022

sbordet left a comment

Choose a reason for hiding this comment

sbordet Dec 19, 2022

Choose a reason for hiding this comment

sbordet Dec 19, 2022

Choose a reason for hiding this comment

gregw Dec 19, 2022

Choose a reason for hiding this comment

sbordet Dec 19, 2022

Choose a reason for hiding this comment

gregw Dec 19, 2022

Choose a reason for hiding this comment

lorban left a comment

Choose a reason for hiding this comment

gregw commented Dec 11, 2022 •

edited by sbordet

Loading