Do not log ClientAbortException

[schicho]

Description

This is a horribly flaky issue and usually only happens when using in Firefox.
Reproducing this consistently is next to impossible.

The issue has been listed multiple times for Mojarra already, namely #547 and #4280

This occurs due to Firefox's behaviour, when sending requests for resources, but canceling/dropping the connection midway through, because the cache was faster. There are 4 possible behaviours as described on the Mozilla support-page. Of the 4 cases, the ClientAbortException only happens in case 4.

Current Behaviour

The ClientAbortConnection is logged as IOException in the method call to send404(context, resourceName, libraryName, ioe, true);

It is logged on level Warning and prints the whole stacktrace into the log.

This Change

The ClientAbortException is no longer logged, as there is no action that can be taken by the server.

Further Details / Questions

• MyFaces handles it already like this. Their implementation.
• How should I handle the translation of the error messages?
• My current change does not abstract away sending the 404 status. I currently opted for the shortest possible fix. Is another approach suited better e.g. handling it inside send404?

[arjantijms]

How should I handle the translation of the error messages?

Translation of log messages is a bit a thing of the past. There's still the intention to remove i18n for log messages entirely.

[codylerum]

Is it right to be matching on org.apache.catalina.connector.ClientAbortException? That isn't a standard thing that is going to be in every container is it?

[arjantijms]

Is it right to be matching on org.apache.catalina.connector.ClientAbortException?

Indeed, that would only work for Tomcat and servers using Catalina (such as GlassFish).

[schicho]

Is it right to be matching on org.apache.catalina.connector.ClientAbortException?

Indeed, that would only work for Tomcat and servers using Catalina (such as GlassFish).

Oh, I was not aware. I am indeed using TomCat, so this is how i stumbled upon this, looking for solutions. MyFaces further checks on another exception, but as I didn't know what it was I left it out, as only checking for ClientAbortException was enough to fix it for me.

private static boolean isConnectionAbort(IOException e)
{
    return e.getClass().getCanonicalName().equals("org.apache.catalina.connector.ClientAbortException")
            || e.getClass().getCanonicalName().equals("org.eclipse.jetty.io.EofException");
}

Should I also check for this?

---

I have also created an eclipse account now and signed the contribution agreements. That check should pass now.

[codylerum]

If this is a common occurrence and there is no action that can be taken by the server then why not just send the 404 and skip the log?

send404(context, resourceName, libraryName, ioe, false);

Not seeing the value of the log.

[codylerum]

I still don't think matching for org.apache.catalina.connector.ClientAbortException makes sense. My thought was just to send the 404 without log for anything in that catch.

@arjantijms what are your thoughts?

[arjantijms]

Not super happy about having product specific things in Mojarra. I have to admit that for customers we have been catching these same exceptions too in proprietary code, so I do understand the wish.

Maybe a better course of action is to standardise this exception in Servlet?

[pizzi80]

Maybe a better course of action is to standardise this exception in Servlet?

in the meantime it could be merged with a comment
"to remove when servlet spec will fill the gap with reality ..."

maybe this OmniFaces utility is related and could be ported / adapted ?

https://showcase.omnifaces.org/exceptionhandlers/ExceptionSuppressor

The docs suggests that these 2 exceptions are also good candidates to be ignored:

java.nio.channels.ClosedByInterruptException
java.nio.channels.IllegalSelectorException

[melloware]

I think you should check the same way MyFaces does for both exceptions...

private static boolean isConnectionAbort(IOException e)
    {
        return e.getClass().getCanonicalName().equals("org.apache.catalina.connector.ClientAbortException")
                || e.getClass().getCanonicalName().equals("org.eclipse.jetty.io.EofException");
    }

[melloware]

See comment

[mnriem]

How about employ what is illustrated in the ExceptionHandler JavaDoc

} catch (Exception e) {
   FacesContext ctx = FacesContext.getCurrentInstance();
   ExceptionQueuedEventContext eventContext = new ExceptionQueuedEventContext(ctx, e);
   eventContext.getAttributes().put("key", "value");
   ctx.getApplication().publishEvent(ExceptionQueuedEvent.class, eventContext);
 }

[melloware]

Whatever we decide i would like to make sure MyFaces does the same thing as Mojarra.

[schicho]

I have for now stuck to the way I did it initially.

Added the check for Eof and added the comment about servlet.

[mnriem]

I do not favor this approach. @arjantijms What do you think about the code snippet I pasted above? Would that not be a more reasonable approach? Note that binding yourself to a specific underlying runtime if it can be avoided it should!

[melloware]

Agree @mnriem a non specific solution would be great and then we can Migrate whatever we come up with to MyFaces. Cc @tandraschko

[schicho]

Regarding

} catch (Exception e) {
   FacesContext ctx = FacesContext.getCurrentInstance();
   ExceptionQueuedEventContext eventContext = new ExceptionQueuedEventContext(ctx, e);
   eventContext.getAttributes().put("key", "value");
   ctx.getApplication().publishEvent(ExceptionQueuedEvent.class, eventContext);
 }

If I understand correctly, according to the docs:

With either approach, any ExceptionQueuedEvent instances that are published in this way are accessible to the {@link #handle} method, which is called at the end of each lifecycle phase, as specified in section 6.2 "ExceptionHandler" of the Jakarta Faces Specification Document.

Looking into the default ExceptionHandler does it not just log the error on level Severe again?
This would not change anything about the current behaviour. Nor be similar to the implementation and log level in MyFaces.

[tandraschko]

yeah
IMO such a "connection close" is nothing that should be handled by the application developer via ExceptionHandler

[schicho]

What is the current status of this PR? I would like to see this move forward, as my logs are still unneccesarily cluttered.
Or is it still on hold for reasons like stated above, i.e. the exception not being standardized?

[melloware]

I am all for it.

[arjantijms]

If needed we can expand the section of well known client abort exceptions. If anyone wants to push for this being standardised, that would be even better.