Jetty 12 immutable ee10 configurations #9724
Conversation
WIP made abstract builder
WIP making more Configurations immutable
|
@janbartel would be good to get review and/or help as I go on this one. |
WIP annotations
WIP don't clone Configurations
WIP don't clone Configurations
…table-ee10-configurations # Conflicts: # jetty-ee10/jetty-ee10-quickstart/src/main/java/org/eclipse/jetty/ee10/quickstart/QuickStartGeneratorConfiguration.java # jetty-ee10/jetty-ee10-webapp/src/main/java/org/eclipse/jetty/ee10/webapp/JaasConfiguration.java
|
@janbartel nudge |
| @@ -68,6 +61,12 @@ public IncludeExcludeSet() | |||
| this(HashSet.class); | |||
| } | |||
|
|
|||
| public IncludeExcludeSet<T, P> asImmutable() | |||
| { | |||
| // TODO | |||
There was a problem hiding this comment.
A new TODO in new code??!
| @@ -176,7 +176,7 @@ public static Configurations setServerDefault(Server server) | |||
| return configurations; | |||
| configurations = getServerDefault(server); | |||
| server.addBean(configurations); | |||
| server.setAttribute(Configuration.ATTR, null); | |||
| server.setAttribute(SERVER_DEFAULT_ATTR, null); | |||
There was a problem hiding this comment.
This is really not server defaults, they are specifically environment defaults. If an ee9 context setup was to call this method, and then an ee10 called the same method, one would overwrite the other with meaningless class references because they are environment classloader specific. I'd rather this was something like:
setEnvironmenDefault(Environment)
There was a problem hiding this comment.
But we can have multiple servers in the same environment and each will have a different set of default configurations. So I think they still are server defaults
| } | ||
| } | ||
|
|
||
| protected AbstractConfiguration(Builder builder) |
There was a problem hiding this comment.
This is a bit of a weird builder pattern. Normally you don't construct the object by passing it a builder, you use the builder to construct the object. Can't these fields just be protected final blank in AbstractConfiguration?
There was a problem hiding this comment.
It is a little different, but I don't think it can be miss-used, so I think it is OK.
| @@ -605,7 +597,7 @@ public void postConfigure(WebAppContext context) throws Exception | |||
| * @throws IOException if unable to scan the directory | |||
| */ | |||
| // TODO: Needs to use resource. | |||
| protected final List<DiscoveredServletContainerInitializerHolder> _sciHolders = new ArrayList<>(); | ||
|
|
||
| protected List<ParserTask> _parserTasks; | ||
| protected static class State |
There was a problem hiding this comment.
I was intended to do that, but I'd need to make all fields final, which is a bit of work that risks changing behaviour.
| } | ||
|
|
||
| @Override | ||
| public void configure(WebAppContext context) throws Exception | ||
| { | ||
| State state = (State)context.getAttribute(STATE); |
| _classInheritanceHandler = null; | ||
| _containerInitializerAnnotationHandlers.clear(); | ||
| _sciHolders.clear(); | ||
| State state = (State)context.removeAttribute(STATE); |
From #9686 making Configurations immutable