search

jboss / mojarra

Fork of Mojarra
10 stars 41 forks source link

JBEAP-16460 Don't log expected NoSuchMethodException after it's caught. #29

Closed soul2zimate closed 5 years ago

soul2zimate commented 5 years ago

Signed-off-by: Chao Wang chaowan@redhat.com (cherry picked from commit c7f4c91c15ef0749e8d68300571c9830b192c483)

issue: https://issues.jboss.org/browse/JBEAP-16460

upstream PR: https://github.com/jboss/mojarra/pull/28 community PR: https://github.com/eclipse-ee4j/mojarra/pull/4545 and https://github.com/eclipse-ee4j/mojarra/pull/4590

wolfc commented 5 years ago

@soul2zimate start by adding a test for the documented contract. Then amend the test for the issue reported. After those I would like to see a potential fix.

soul2zimate commented 5 years ago

I agree that this shouldn't not be removed. Since https://github.com/eclipse-ee4j/mojarra/blob/master/impl/src/main/java/com/sun/faces/spi/ServiceFactoryUtils.java#L59-L69 really takes care of it for exception catching.

       try {
            Class<?> clazz = Util.loadClass(entry, null);
            Constructor c = clazz.getDeclaredConstructor(argumentTypes);
            if (c == null) {
                throw new FacesException("Unable to find constructor accepting arguments: " + Arrays.toString(arguments));
            }
            return c.newInstance(arguments);
        } catch (ClassNotFoundException | NoSuchMethodException | SecurityException | FacesException | InstantiationException | IllegalAccessException | IllegalArgumentException | InvocationTargetException e) {
            if (LOGGER.isLoggable(Level.FINE)) {
                LOGGER.log(Level.FINE, e.toString(), e);
            }
            return null;
        }

We only know whether or not customer AnnotationProvider support two parameters construct when ServiceFactoryUtils checks and creates new instance.

The potential fix I see here is just lowering the log level for NoSuchMethodException when it's caught. This stops printing those log messages in WildFly with TRACE level enabled. Also this still allows upstream JDKLogger to see it in Level.FINER level.

fjuma commented 5 years ago

@soul2zimate I don't think it makes sense to treat the NoSuchMethodException differently from the other exceptions that could be thrown. The presence of the message in the log with TRACE enabled isn't a bug. It just indicates that the two argument constructor wasn't found. I think that using Level.FINE is actually ok here.

soul2zimate commented 5 years ago

Thanks @fjuma,Do you mind if I put your opinion in WFLY-6918 / JBEAP-16460 and resolve as NOT A BUG in that case ?

wolfc commented 5 years ago

@fjuma but the presence of the other exceptions indicates an unexpected problem. These should not be logged at all, but rather rethrown. The NoSuchMethodException should not appear at all unless neither constructor type is found, in which case it might even be more prudent to make it a RuntimeException or FacesException as again an unexpected problem has occurred.

fjuma commented 5 years ago

Took a closer look at this. It appears that the Mojarra team lowered the logging level from SEVERE to FINE a while back in order to try to make the noise in the logs less confusing, as shown in the following commit:

https://github.com/jboss/mojarra/commit/0ed376ff29cf6846cdee4d3f184ed8b411d04897

I suspect they wanted to log exceptions here since they had designed this method to return null if the constructor could not be found or if any issues occurred while attempting to use it. In these cases, the message would be logged and the second constructor would get used successfully.

If the presence of the message in the log with TRACE enabled is still confusing, the ServiceFactory.getProviderFromEntry method could be modified to wrap any exceptions that occur in a FacesException and throw that instead of logging the exceptions.

The AnnotationProviderFactory.createAnnotationProvider method would then need to be updated to catch this exception if it occurs when calling the two-argument constructor and then attempt to use the one-argument constructor. This means that if neither constructor can be used, AnnotationProviderFactory.createAnnotationProvider will now throw a FacesException instead of returning null. However, it seems that all the usages of AnnotationProviderFactory.createAnnotationProvider already implicitly assume that the returned value is non-null anyway, so this change should be ok.

soul2zimate commented 5 years ago

I have rebased this to handle those exceptions differently, rethrow FacesException for serious problems and ignore the expected NoSuchMethodException

xstefank commented 5 years ago

@fjuma can you please take a look again please?

fjuma commented 5 years ago

Looks ok now but we should get this approved upstream as well.

soul2zimate commented 5 years ago

https://github.com/eclipse-ee4j/mojarra/pull/4545 is merged.

fjuma commented 5 years ago

@soul2zimate Thanks for the update. Since the corresponding JBEAP issue is fully acked and since the upstream PR has been merged, SET can go ahead and merge this when ready.

soul2zimate commented 5 years ago

Merged, Hi @fjuma, you want me to merge https://github.com/jboss/mojarra/pull/28 as well ?

fjuma commented 5 years ago

Yes, that would be great. Thanks @soul2zimate!