eclipse-ee4j / mojarra

Mojarra, a Jakarta Faces implementation
Other
160 stars 109 forks source link

SystemEventListener executed twice if faces-config.xml is found by multiple config providers. #5188

Closed larsgrefer closed 1 year ago

larsgrefer commented 1 year ago

Describe the bug

I've found an interesting corner case where system event listeners like org.primefaces.webapp.PostConstructApplicationEventListener may be executed twice.

In my application I have a custom com.sun.faces.spi.ConfigurationResourceProvider which also finds META-INF/faces-config.xml entries in the classpath.

This causes primefaces-12.0.0-jakarta.jar!/META-INF/faces-config.xml (and others) to be found twice by com.sun.faces.config.manager.Documents#getXMLDocuments.

This - in turn - causes the listeners to be instantiated and executed twice.

Expected behavior

I'd expect that the same faces-config.xml file should not be processed twice if found by multiple ConfigurationResourceProviders

Additional context

Java 17.0.5 Mojarra 4.0.0 Tomcat 10.1.1

larsgrefer commented 1 year ago

Something like this may fix it:

 impl/src/main/java/com/sun/faces/config/manager/Documents.java | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/impl/src/main/java/com/sun/faces/config/manager/Documents.java b/impl/src/main/java/com/sun/faces/config/manager/Documents.java
index 7b3676b9..aa90ce98 100644
--- a/impl/src/main/java/com/sun/faces/config/manager/Documents.java
+++ b/impl/src/main/java/com/sun/faces/config/manager/Documents.java
@@ -24,7 +24,9 @@ import static java.util.logging.Level.INFO;
 import java.net.URI;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.FutureTask;
@@ -83,12 +85,15 @@ public class Documents {
         }

         // Load and XML parse all documents to which the URLs that we collected above point to
-
+        Set<URI> processedUris = new HashSet<>();
         List<FutureTask<DocumentInfo>> docTasks = new ArrayList<>(providers.size() << 1);

         for (FutureTask<Collection<URI>> uriTask : uriTasks) {
             try {
                 for (URI uri : uriTask.get()) {
+                    if (!processedUris.add(uri)) {
+                        continue;
+                    }
                     FutureTask<DocumentInfo> docTask = new FutureTask<>(new ParseConfigResourceToDOMTask(servletContext, validating, uri));
                     docTasks.add(docTask);
melloware commented 1 year ago

@larsgrefer that seems reasonable to me!

BalusC commented 1 year ago

I'm not sure. This will hide potential user errors later on. E.g. accidentally having duplicate Mojarra impls or duplicate PrimeFaces libs in the runtime classpath. It would then not anymore be obvious from the system logs that the libs are messed up.

Why exactly do you have a second com.sun.faces.spi.ConfigurationResourceProvider? Why not just overriding Mojarra's own one?

larsgrefer commented 1 year ago

I'm not sure. This will hide potential user errors later on. E.g. accidentally having duplicate Mojarra impls or duplicate PrimeFaces libs in the runtime classpath. It would then not anymore be obvious from the system logs that the libs are messed up.

I don't think this would happen. If I had multiple Primefaces libs on the classpath, their URI's would be different so they'd still be executed twice.

Why exactly do you have a second com.sun.faces.spi.ConfigurationResourceProvider? Why not just overriding Mojarra's own one?

How would it be possible to override Mojarra's own one? ConfigManager always adds the 3 default providers like com.sun.faces.config.configprovider.MetaInfFacesConfigResourceProvider. It seems like I can only add more providers but not replace the default ones.

BalusC commented 1 year ago

I don't think this would happen. If I had multiple Primefaces libs on the classpath, their URI's would be different so they'd still be executed twice.

Ok.

It seems like I can only add more providers but not replace the default ones.

Let's take a step back. Which problem exactly were you trying to solve this way? I can't ignore the impression that this can be done in a much more clean way.

larsgrefer commented 1 year ago

I'm already performing some classpath scanning in my application, so the idea was to re-use the results for Mojarra by implementing the SPI's. For this specific case, the fix might be just to remove my ConfigurationResourceProvider implementation as it brings no benefit at the moment because MetaInfFacesConfigResourceProvider is executed nonetheless.

That being said, I'd still argue that it should not be a problem if two or more ConfigurationResourceProviders find the exact same faces-config.xml