Compliance Violation Listener cleanup of alternate proposal #11301

joakime · 2024-01-22T21:01:52Z

Additional clean of PR #11283 (Alternate proposal of Compliance Violations Listener handling)

joakime · 2024-01-22T23:19:24Z

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java

@@ -363,4 +359,62 @@ private static Set<Violation> copyOf(Set<Violation> violations)
            return EnumSet.noneOf(Violation.class);
        return EnumSet.copyOf(violations);
    }
+
+    public static void checkHttpCompliance(MetaData.Request request, HttpCompliance mode,


Used by HTTP/2 and HTTP/3

joakime · 2024-01-22T23:19:41Z

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java

-    /**
-     * A Listener that represents multiple user {@link ComplianceViolation.Listener} instances
-     */
-    class ListenerCollection implements Listener


Moved to AbstractConnector (as it was the only place using it)

gregw

So far so good....

gregw · 2024-01-22T23:33:40Z

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

@@ -85,12 +85,14 @@ public Runnable onRequest(HeadersFrame frame)
        {
            _requestMetaData = (MetaData.Request)frame.getMetaData();

-            ComplianceViolation.Listener listener = Server.getComplianceViolationListener(_httpChannel.getConnectionMetaData().getConnector()).initialize();
+            // Grab freshly initialized ComplianceViolation.Listener here, no need to reinitialize.
+            ComplianceViolation.Listener listener = _httpChannel.getComponents().getComplianceViolationListener();


@sbordet This is the exposed violations listeners in components. I agree it is kind of funny... but is it really any different to the exposed HttpConfiguration (see change below)?

The HttpConfiguration is public API, but also has getters for lists of Customizers, which users are not meant to use directly.

What if we moved the compliance listeners to HttpConfiguration? They'd still be able to be associated with a particular connector if desired? It would also be very much more explicit rather than just adding beans.

If we move the configuration of ComplianceViolation.Listener (from being beans on Connector or Server) to HttpConfiguration then that simplifies a bunch of things too.

We could have methods:

addComplianceViolationListener(ComplianceViolation.Listener)

removeComplianceViolationListener(ComplianceViolation.Listener)

getComplianceViolationListeners() - returns the list of listeners

getActiveComplianceViolationListener() - returns the active (possibly composite, possible NOOP) ComplianceViolation.Listener suitable to use in the code.

The existence of any added listener would be the equivalent of HttpConfiguration.isNotifyComplianceViolations() of true (no need for this boolean).
We could also get rid of Server.getComplianceViolationListener(Connector connector) in favor of just using the HttpConfiguration.getActiveComplianceViolationListener().

This however, wouldn't get rid of, or change, the existing storage and management from HttpChannelState tho. right?
We still need the initialize() / onBeginRequest() / onEndRequest() calls from the right places in HTTP/1, HTTP/2, and HTTP/3

gregw · 2024-01-22T23:36:04Z

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

@@ -258,6 +258,7 @@ private HttpChannel pollHttpChannel()
            httpChannel = httpChannels.poll();
        if (httpChannel == null)
            httpChannel = httpChannelFactory.newHttpChannel(this);
+        httpChannel.init();


I know I first used the "initialize" verb... but is it really the right verb? perhaps prepare?

Anyway, failing a better name, can this at least be initialize

gregw · 2024-01-22T23:38:15Z

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java

+        // TODO: it's a bit odd to hang onto a ComplianceViolation.Listener here.
+        // as that would require the user to pass it in.  Maybe a List<Event> violations instead?
+        // that the ee10 layer could use?
+        private ComplianceViolation.Listener complianceViolationListener;


I'm -1 on keeping lists of violations just in case later on there is a violation listener we need to pass it too.

Just have this field with an optional constructor. If it is constructed without the listener, then there is no listener.

gregw · 2024-01-22T23:40:35Z

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

@@ -58,18 +57,27 @@ public HttpConfiguration getHttpConfiguration()
    @Override
    public void configure(Connector connector)
    {
-        if (isRecordHttpComplianceViolations())
+        // TODO: need HTTP/2 and HTTP/3 version of this
+        if (getHttpConfiguration().isNotifyComplianceViolations())


Yeah this is screaming to me now to just add the listeners to the HttpConfiguration. Why look at the configuration to decide if to add a bean that is looked for elsewhere.

gregw · 2024-01-22T23:43:35Z

I've closed #11283 in favour of this PR. But I'll repeat my (updated) review response from there here so it is read:

@sbordet I'm a hard -1 on an attribute/bean to lookup the listeners. It is just in too many places. With a field, then the JIT has some chance of optimizing for a null or constant value, but with an attribute we will be looking in a map (probably behind a wrapper behind a lazy atomic reference) every time. It is used too frequently in too many places to put into just an attribute.

I agree that it is a little strange to "initialize" the listener in the "recycle" step, but that is because there is no even before onRequest that can be used to initialize the listener for any violations that occur before there is a request. I could add a initialize event to the HttpChannel, but that feels more intrusive..... hmmm but that would solve the creating a new listener for a channel that would not be used again.... I'll investigate.... oh look @joakime did that already in this PR!

I also kind of agree that Components is not exactly the right place to surface this. HttpChannelState is the right place... but then we do not have a common way to navigate from Components to HttpChannelState... other than down casting Components. Perhaps that will be sufficient for this? OR perhaps we just move the listeners to HttpConfiguration?

joakime · 2024-01-23T06:58:54Z

Note: the original PR #11254 has been updated with the changes from proposal 2 (gregs) and 3 (this one).

gregw · 2024-01-23T12:24:20Z

@joakime Ah no! We still need to have the listeners as a field on the HttpChannelState, as that is where the lifecycle is. If the combined listener is on the HttpConfiguration, then we can't call init on it.

I don't mind moving the the listeners to HttpConfiguration anyway.... it just doesn't solve the problem of moving them out of Components.

gregw · 2024-01-23T12:25:51Z

closed in favor of #11254

gregw · 2024-01-23T12:59:33Z

@joakime I have pushed a change to move the listeners to config, but still with the access via Components. I'm struggling to find a better way than that.

Cleanup and reorganization of Proposal 2

dc5de07

joakime added Bug For general bugs on Jetty side Sponsored This issue affects a user with a commercial support agreement labels Jan 22, 2024

joakime requested review from gregw and sbordet January 22, 2024 21:01

joakime self-assigned this Jan 22, 2024

joakime commented Jan 22, 2024

View reviewed changes

gregw mentioned this pull request Jan 22, 2024

Alternate Fix/12.0.x/cookie compliance logging2 #11283

Closed

gregw reviewed Jan 22, 2024

View reviewed changes

joakime mentioned this pull request Jan 23, 2024

Issue #11253 - Cleanup ComplianceViolation behavior to allow Cookie / URI / MultiPart compliance to also receive listener events. #11254

Merged

gregw closed this Jan 23, 2024

joakime deleted the fix/12.0.x/cookie-compliance-logging3 branch February 28, 2024 13:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compliance Violation Listener cleanup of alternate proposal #11301

Compliance Violation Listener cleanup of alternate proposal #11301

joakime commented Jan 22, 2024

joakime Jan 22, 2024

joakime Jan 22, 2024

gregw left a comment

gregw Jan 22, 2024

joakime Jan 23, 2024 •

edited

Loading

gregw Jan 22, 2024

gregw Jan 22, 2024

gregw Jan 22, 2024 •

edited

Loading

gregw commented Jan 22, 2024

joakime commented Jan 23, 2024

gregw commented Jan 23, 2024

gregw commented Jan 23, 2024

gregw commented Jan 23, 2024

Compliance Violation Listener cleanup of alternate proposal #11301

Compliance Violation Listener cleanup of alternate proposal #11301

Conversation

joakime commented Jan 22, 2024

joakime Jan 22, 2024

Choose a reason for hiding this comment

joakime Jan 22, 2024

Choose a reason for hiding this comment

gregw left a comment

Choose a reason for hiding this comment

gregw Jan 22, 2024

Choose a reason for hiding this comment

joakime Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

gregw Jan 22, 2024

Choose a reason for hiding this comment

gregw Jan 22, 2024

Choose a reason for hiding this comment

gregw Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

gregw commented Jan 22, 2024

joakime commented Jan 23, 2024

gregw commented Jan 23, 2024

gregw commented Jan 23, 2024

gregw commented Jan 23, 2024

joakime Jan 23, 2024 •

edited

Loading

gregw Jan 22, 2024 •

edited

Loading