Issue #11514 - Cleanup `jetty.webapp.addServerClasses` property behavior for ee10/ee9/ee8 #11516

joakime · 2024-03-13T17:05:49Z

Cleanup of how jetty.webapp.addServerClasses and jetty.webapp.addSystemClasses properties work in Jetty 12.

Standardize all storage of configurations in the associated Environment attributes (we had some in Environment, some in Server)
Deprecate static methods that take the Attributes or Server classes (as those were out of sync with what WebAppContext.setServer(Server) behavior was working.
Add testing of new WebAppContext.addServerClasses(String)

This issue was discovered during the cross context dispatch testing, and is a critical piece of that testing.

…ior for ee10/ee9/ee8

joakime · 2024-03-13T17:06:47Z

...0/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java

+    public static final Environment ENVIRONMENT = Environment.ensure("ee10");
+    /**
+     * @deprecated Use {@link ServletContextHandler#ENVIRONMENT} instead. will be removed in Jetty 12.1.0.
+     */
+    @Deprecated(since = "12.0.8", forRemoval = true)
+    public static final Environment __environment = ENVIRONMENT;


This is just standardizing the public constant to the same way we have it in ee9/ee8.

sbordet

I really don't like specifying future, possibly will-never-exist, Jetty versions in the code.
Other than that, LGTM.

sbordet · 2024-03-18T08:30:58Z

...0/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java

-    public static final Environment __environment = Environment.ensure("ee10");
+    public static final Environment ENVIRONMENT = Environment.ensure("ee10");
+    /**
+     * @deprecated Use {@link ServletContextHandler#ENVIRONMENT} instead. will be removed in Jetty 12.1.0.


Remove the part related to when it will be removed.

I disagree, this is super common in use on open source java projects (spring-boot, spring-framework, dropwizard, tomcat, etc) and benefits developers.
Our own codebase already has many such usages before this PR.

@joakime then we should remove them all.

Spring has a policy (2 minor versions).

We do not know if we ever will have a 12.1.0, maybe we jump to 13 immediately due to the move to Java 21 required by JEE 11.

Or maybe we will have a policy similar to Spring.

Or maybe we retain it for sponsored reasons.

So the comment about 12.1.0 is wrong, in all cases, and must be removed.

I'm on the fence here. I like seeing the deprecated for removal. I also don't mind being given info that will let me know I can keep using it (e.g. knowing it will always be there in 12.0.x). But I also can see that naming a specific future version is not always correct (although we have frequently done it before and survived!).

So perhaps just say it is deprecated for removal "after 12.0.x", and that could be 12.1.0 or 13.0.0

So the comment about 12.1.0 is wrong, in all cases, and must be removed.

I disagree with the "must be removed" part of your reply.
but I'm totally on board with having a minimum 2 minor releases policy.
That would change these references to say "12.2.0" instead, which means even if 13.0.0 gets released, they are removed, as 13.0.0 is after 12.2.0.

@joakime then the policy is outlined elsewhere, e.g. in the docs.
Leave the code clean, without comments that are going to rot.

The policy on deprecated doesn't exist in any documentation in Jetty.

Sure, let's add it, then. But it is orthogonal to this issue.

sbordet · 2024-03-18T08:32:54Z

jetty-ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/WebAppContext.java

+    /**
+     * @param server ignored.
+     * @param patterns the patterns to add
+     * @deprecated use {@link #addServerClasses(String...)} instead, will be removed in Jetty 12.1.0


Remove when it will be removed.

sbordet · 2024-03-18T08:33:21Z

jetty-ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/WebAppContext.java

+    /**
+     * @param server ignored.
+     * @param patterns the patterns to add
+     * @deprecated use {@link #addSystemClasses(String...)} instead, will be removed in Jetty 12.1.0


Remove when it will be removed.

sbordet · 2024-03-18T08:41:58Z

jetty-ee9/jetty-ee9-webapp/src/main/java/org/eclipse/jetty/ee9/webapp/WebAppContext.java

+    /**
+     * @param attributes ignored.
+     * @param patterns the patterns to add
+     * @deprecated use {@link #addServerClasses(String...)} instead. will be removed in Jetty 12.1.0


Remove when it will be removed.

sbordet · 2024-03-18T08:42:08Z

jetty-ee9/jetty-ee9-webapp/src/main/java/org/eclipse/jetty/ee9/webapp/WebAppContext.java

+    /**
+     * @param attributes ignored.
+     * @param patterns the patterns to add
+     * @deprecated use {@link #addSystemClasses(String...)} instead. will be removed in Jetty 12.1.0


Remove when it will be removed.

gregw · 2024-03-21T06:25:22Z

jetty-ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/WebAppContext.java

+    /**
+     * @param server ignored.
+     * @param patterns the patterns to add
+     * @deprecated use {@link #addServerClasses(String...)} instead, will be removed in Jetty 12.1.0


The method that doesn't take a server does a different job. That sets the default for all servers rather than this method, which sets the default for a specific server.

What is the replacement for setting the default for a specific server? That should be linked here.

That's at the heart of what caused the issue.

The Attributes reference passed to addServerClasses was not used by the WebAppContext.setServer(Server server) call that needs the configuration.

Now with this PR the add(Server|System)Classes and setServer(Server) both use the environment specific Attributes locations.
Which works great, and even allows for wacky setups where 1 environment has a different setup than a different environment.

Do we have something like a jetty-core level WebAppContext (or something) that can be used as the Server level one?
I'm on the fence with the idea of having a Server.add(Server|System)Classes method which only has a List<String> (not the ClassMatcher option), but it feels like the wrong place for it.
Interestingly, while working this, I found a bug in the ee8 layer's XML where it attempted to use the Server instance to call WebAppContext.addServerClasses(Server, String...), but due to XML order that Server didn't exist yet, which was simply worked around by using the Environment Attributes instead.

How about in a followup PR I move the 2 ClassMatcher impls (from ee10 & ee9) to jetty-util, and then put the Attributes neutral version in the new jetty-util code?

@joakime note that the server classes are set on a static constant ServletContextHandler.ENVIRONMENT, so if you have 2 Servers in the same JVM, and you want different serverClasses configuration, you cannot.

The ServletContextHandler.ENVIRONMENT (that exists in ee10, ee9, ee8) are JVM statics ATM (in HEAD).
I think changing that is out of scope for this PR, but I can see that possibly being a useful concept in the long run.

sbordet · 2024-03-21T15:51:59Z

jetty-ee9/jetty-ee9-webapp/src/main/java/org/eclipse/jetty/ee9/webapp/WebAppContext.java

@@ -1504,6 +1543,6 @@ private static void addClasses(ClassMatcher matcher, String attribute, Attribute
        int l = classes.length;
        classes = Arrays.copyOf(classes, l + pattern.length);
        System.arraycopy(pattern, 0, classes, l, pattern.length);
-        attributes.setAttribute(attribute, classes);
+        ServletContextHandler.ENVIRONMENT.setAttribute(attributeKey, classes);


Why cannot system and server classes be a non-static field in WebAppContext?

The attributes are only used internally by WebAppContext, so there is no need to store them into a static constant, which will cause troubles in case of multiple Server instances in the same JVM.

This method is used for configuring all WebAppContext's that get created for this environment in a common way.
There is already WebAppContext instance specific ways of configuring the ClassMatcher used by the WebApp.
Take a look at WebAppContext.setServer(Server) for where the instance specific ClassMatcher is configured.

gregw

I agree there is a problem and also that perhaps there is something more that we should do about configuring for an ENVIRONMENT, but I don't think we should give up configuring for a single server.

Other than the clean up, what was the actual bug that stopped EE8/EE9 seeing the server classes? Edit: sorry just saw your comment... reading your answer now....

jetty-ee10/jetty-ee10-webapp/src/main/config/etc/jetty-ee10-webapp.xml

jetty-ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/WebAppContext.java

gregw · 2024-03-23T17:22:08Z

@joakime thanks for your comment (which I initially missed). It has explained a lot.

You have definitely found a problem and are on the way to a solution, but there are a couple of concepts that are not correct:

Currently (and with your PR), we have in each EE WebAppContext:

    // System classes are classes that cannot be replaced by
    // the web application, and they are *always* loaded via
    // system classloader.
    public static final ClassMatcher __dftSystemClasses = new ClassMatcher(
        "java.",                            // Java SE classes (per servlet spec v2.5 / SRV.9.7.2)
        "javax.",                           // Java SE classes (per servlet spec v2.5 / SRV.9.7.2)
        "jakarta.",                         // Jakarta classes (per servlet spec v5.0 / Section 15.2.1)
        "org.xml.",                         // javax.xml
        "org.w3c."                          // javax.xml
    );

    // Server classes are classes that are hidden from being
    // loaded by the web application using system classloader,
    // so if web application needs to load any of such classes,
    // it has to include them in its distribution.
    public static final ClassMatcher __dftServerClasses = new ClassMatcher(
        "org.eclipse.jetty."                // hide jetty classes
    );

But because these statics are on EE classes, they are not server wide. They are effectively only in the Environment that loads the class, so they are Env defaults not server defaults.
Switching to use the Environment as the Attributes is not really the right thing to do, as these attributes have exactly the same scope as the statics on the WebAppContext classes.

There is also the issue that EE10 WebAppContext uses:

    public static final String SERVER_SYS_CLASSES = "org.eclipse.jetty.webapp.systemClasses";
    public static final String SERVER_SRV_CLASSES = "org.eclipse.jetty.webapp.serverClasses";

Whilst EE9 WebAppContext uses names scoped to the environment:

    public static final String SERVER_SYS_CLASSES = "org.eclipse.jetty.ee9.webapp.systemClasses";
    public static final String SERVER_SRV_CLASSES = "org.eclipse.jetty.ee9.webapp.serverClasses";

Having an attribute name scope to an environment inside an environment scoped Attributes is at least duplicitous if not an anti-pattern.

So you have definitely identified a bit of a mess! But I think we need bigger steps to fix it than what you have here. However, I think you are on the right course when you say:

move the 2 ClassMatcher impls (from ee10 & ee9) to jetty-util, and then put the Attributes neutral version in the new jetty-util code

I think we should do that now (or perhaps to a jetty-ee-common module )and build up from there so we can have system/server classes specified for:

all servers in the JVM (stored as ClassMatcher static field on some jetty-ee-common class)
a specific server (stored as an attribute on the Server instance)
a specific environment (stored either as attribute on the Environment or as a ClassMatcher static field on the EE specific WebAppContext class
a specific context (stored as a ClassMatcher field on the EE specific WebAppContext class

We will need the jetty-eeX-webapp modules to depend on a jetty-ee-webapp module

This is a bit of work, but we have a few days to get it done for this release cycle.... if we don't finish, we can bump to the next.

Other than that, the cleanups of naming etc. in this PR are good.

gregw · 2024-03-23T17:57:45Z

@joakime I've started a branch where I'm moving a few files from eeX to a new jetty-core/jetty-ee module. Give me a day or two to progress that, then you can perhaps merge with your branch and use it...

gregw · 2024-03-24T18:49:14Z

@joakime my first pass at this is here: org.eclipse.jetty.ee.WebappProtectedClasses probably a bit more work needed.

Edit: I mean fix/12.0.x/addserverclasses-ee9-gw

gregw · 2024-03-25T10:41:17Z

See #11566 with ClassMatcher in util and a common ee module

joakime · 2024-04-15T16:36:43Z

Closing as PR #11566 completed this need

joakime added 2 commits March 13, 2024 12:02

Issue #11514 - Cleanup jetty.webapp.addServerClasses property behav…

c8c6dad

…ior for ee10/ee9/ee8

Fix test

503225e

joakime added the Bug For general bugs on Jetty side label Mar 13, 2024

joakime requested a review from gregw March 13, 2024 17:05

joakime self-assigned this Mar 13, 2024

joakime commented Mar 13, 2024

View reviewed changes

joakime marked this pull request as draft March 13, 2024 17:11

Merging patterns (default -> env -> config)

87d0c45

joakime marked this pull request as ready for review March 13, 2024 18:48

This was referenced Mar 14, 2024

Enable legacy URI mode in Jetty and Servlets airlift/airlift#1127

Merged

Reintroduce Cross Context Dispatch in Jetty 12 #11451

Merged

sbordet requested changes Mar 18, 2024

View reviewed changes

gregw requested changes Mar 21, 2024

View reviewed changes

sbordet self-requested a review March 21, 2024 15:50

sbordet requested changes Mar 21, 2024

View reviewed changes

gregw requested changes Mar 23, 2024

View reviewed changes

jetty-ee10/jetty-ee10-webapp/src/main/config/etc/jetty-ee10-webapp.xml Show resolved Hide resolved

jetty-ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/WebAppContext.java Show resolved Hide resolved

gregw mentioned this pull request Mar 25, 2024

Move ClassMatcher to core to have a consistent fix for addServerClasses in all environments #11566

Merged

joakime closed this Apr 15, 2024

konstantin-mikheev mentioned this pull request Apr 23, 2024

NPE in error handling leading to 100% CPU #11631

Closed

joakime deleted the fix/12.0.x/addserverclasses-ee9 branch April 23, 2024 18:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #11514 - Cleanup `jetty.webapp.addServerClasses` property behavior for ee10/ee9/ee8 #11516

Issue #11514 - Cleanup `jetty.webapp.addServerClasses` property behavior for ee10/ee9/ee8 #11516

joakime commented Mar 13, 2024

joakime Mar 13, 2024

sbordet left a comment •

edited

Loading

sbordet Mar 18, 2024

joakime Mar 18, 2024

sbordet Mar 18, 2024

gregw Mar 21, 2024

joakime Mar 21, 2024

sbordet Mar 21, 2024

joakime Mar 21, 2024

sbordet Mar 21, 2024

sbordet Mar 18, 2024

sbordet Mar 18, 2024

sbordet Mar 18, 2024

sbordet Mar 18, 2024

gregw Mar 21, 2024

joakime Mar 21, 2024

sbordet Mar 21, 2024

joakime Mar 23, 2024

sbordet Mar 21, 2024

joakime Mar 23, 2024 •

edited

Loading

gregw left a comment •

edited

Loading

gregw commented Mar 23, 2024

gregw commented Mar 23, 2024

gregw commented Mar 24, 2024 •

edited

Loading

gregw commented Mar 25, 2024

joakime commented Apr 15, 2024

Issue #11514 - Cleanup jetty.webapp.addServerClasses property behavior for ee10/ee9/ee8 #11516

Issue #11514 - Cleanup jetty.webapp.addServerClasses property behavior for ee10/ee9/ee8 #11516

Conversation

joakime commented Mar 13, 2024

Choose a reason for hiding this comment

sbordet left a comment • edited Loading

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

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

joakime Mar 23, 2024 • edited Loading

Choose a reason for hiding this comment

gregw left a comment • edited Loading

Choose a reason for hiding this comment

gregw commented Mar 23, 2024

gregw commented Mar 23, 2024

gregw commented Mar 24, 2024 • edited Loading

gregw commented Mar 25, 2024

joakime commented Apr 15, 2024

Issue #11514 - Cleanup `jetty.webapp.addServerClasses` property behavior for ee10/ee9/ee8 #11516

Issue #11514 - Cleanup `jetty.webapp.addServerClasses` property behavior for ee10/ee9/ee8 #11516

sbordet left a comment •

edited

Loading

joakime Mar 23, 2024 •

edited

Loading

gregw left a comment •

edited

Loading

gregw commented Mar 24, 2024 •

edited

Loading