[3.1] [WELD-2664] Don't wrap ImmediateInstanceFactory in WeldInstanceFactory. #2296
Conversation
| @@ -54,7 +58,10 @@ public void handleDeployment(DeploymentInfo deploymentInfo, ServletContext servl | |||
| // Listener injection | |||
| for (ListenerInfo listener : deploymentInfo.getListeners()) { | |||
| UndertowLogger.LOG.installingCdiSupport(listener.getListenerClass()); | |||
| listener.setInstanceFactory(WeldInstanceFactory.of(listener.getInstanceFactory(), servletContext, listener.getListenerClass())); | |||
| InstanceFactory<? extends EventListener> instanceFactory = listener.getInstanceFactory(); | |||
| if (!(instanceFactory instanceof ImmediateInstanceFactory)) { | |||
There was a problem hiding this comment.
@darranl so, when exactly is ImmediateInstanceFactory used? Looking at UT code (which I don't know so I might be wrong), it looks like it's used for any dynamic component. That would mean this change is rather aggressive and will result in leaving all such instance creation to UT instead of trying to let Weld do it. Or am I missing something?
There was a problem hiding this comment.
Outside of test cases the only class I see actually using this is ServletContextImpl which used it in three locations.
https://github.com/undertow-io/undertow/blob/master/servlet/src/main/java/io/undertow/servlet/spec/ServletContextImpl.java#L540
https://jakarta.ee/specifications/platform/8/apidocs/javax/servlet/servletcontext#addServlet-java.lang.String-javax.servlet.Servlet-
https://github.com/undertow-io/undertow/blob/master/servlet/src/main/java/io/undertow/servlet/spec/ServletContextImpl.java#L632
https://jakarta.ee/specifications/platform/8/apidocs/javax/servlet/servletcontext#addFilter-java.lang.String-javax.servlet.Filter-
https://github.com/undertow-io/undertow/blob/master/servlet/src/main/java/io/undertow/servlet/spec/ServletContextImpl.java#L734
https://jakarta.ee/specifications/platform/8/apidocs/javax/servlet/servletcontext#addListener-T-
So on the Undertow side each of these three points the API is registering instances of the relevant types, as the API talks about instances I don't think it is appropriate to be discarding and recreating the instances.
Within the subsystem this is used within WebHostService where it is checked if an instance already exists to be used:
Then the other place it is used is UndertowDeploymentInfoService which is where the error being investigated originated.
Firstly:
https://github.com/wildfly/wildfly/blob/master/undertow/src/main/java/org/wildfly/extension/undertow/WebHostService.java#L81
The LogoutSessionListener here has been passed an instance of AuthentiationManager so would not be correct to recreate.
https://github.com/wildfly/wildfly/blob/master/undertow/src/main/java/org/wildfly/extension/undertow/deployment/UndertowDeploymentInfoService.java#L889
I haven't traced this one but the instances of ServletContainerInitializer already exist so it feels it would be odd to recreate.
https://github.com/wildfly/wildfly/blob/master/undertow/src/main/java/org/wildfly/extension/undertow/deployment/UndertowDeploymentInfoService.java#L1080
This is another instance relying on existing state so recreating if it was possible would loose access to that state.
Overall all the instances I can find the ImmediateInstanceFactory is wrapping a previously instantiated instance which potentially has it's own state and in some cases backed by the API documentation which appears to be talking about registering instances.
|
FTR, if we decide to accept this, we'll need the same PR for 4.x version. |
darranl
changed the title
|
@fl4via Would it be possible for you to please take a look at this PR? I have listed above all the places I find within Undertow that use the ImmediateInstanceFactory. |
|
I am planning to merge this soon-ish (tomorrow most likely) as I will be doing an SP1 release round to include the class defining fixes. |
Undertow uses the ImmediateInstanceFactory where a pre-instantiated instance of the Object should be used so this should not be wrapped in a WeldInstanceFactory as that attempts to create a second instance.
https://issues.redhat.com/browse/WELD-2664