Uploaded image for project: 'MINA'
  1. MINA
  2. DIRMINA-941

DefaultIoFilterChain (or any other class) should not catch Throwable without re-throwing

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.5, 2.0.7
    • Fix Version/s: 2.0.8
    • Component/s: Core
    • Labels:
      None
    • Environment:
      N/A

      Description

      I spent several hours tracking down a bug that was ultimately caused by an old slf4j*-api.jar in my encompassing project's classpath. The critical error message ("... method not found... isTraceEnabled()...") was silenced by the "catch Throwable" behavior in the org.apache.mina.core.filterchain.DefaultIoFilterChain class. Since the problem was related to the logging classes, the error wasn't logged, and unfortunately, the runtime exception was not allowed to propagate to the console... Instead it was consumed in DefaultIoFilterChain. This left absolutely no clues for debugging why the SSH server just stopped in the middle of key negotiation.

      The only evidence of the problem was that the SSHD server I was embedding just stopped in the middle of the key exchange. In order to finally find the problem, I had to track down the last known working part of code in the ssh-core package, and edit your source code to catch and print Throwables. While I appreciate that this was possible because of your open source, it was beyond the normal programmer's expectation for embedding a library. In my google searches for the last printed SSH client debug state message ("debug1: SSH2_MSG_KEXINIT sent") , I found several people over the last few years who have struggled for answers at the same point in the code when using Apache's Mina/sshd. There's no indication that many of them succeeded in tracking down the problem. (I'll go back and leave some breadcrumbs for future travelers, where I have logins.)

      My overall point is that it's not nice to catch Throwables.

        Activity

        Hide
        janicki Chris Janicki added a comment -

        That's great, thanks!

        Show
        janicki Chris Janicki added a comment - That's great, thanks!
        Hide
        elecharny Emmanuel Lecharny added a comment -

        Fixed with commit 1c95e0ddfce293abaa0821ebecba17ab84951fed, commit 232bff322f0278f614b8b35545655d94465b9e7a, commit 404352c427a92efb2b80d2e5b7d5bb84693c7a7c and commit f04184d9f2ddb9694a38dbd183af7ebcec538e1e

        Show
        elecharny Emmanuel Lecharny added a comment - Fixed with commit 1c95e0ddfce293abaa0821ebecba17ab84951fed, commit 232bff322f0278f614b8b35545655d94465b9e7a, commit 404352c427a92efb2b80d2e5b7d5bb84693c7a7c and commit f04184d9f2ddb9694a38dbd183af7ebcec538e1e
        Hide
        elecharny Emmanuel Lecharny added a comment -

        I removed all the places where we were doing a catch (Throwable) and replaced it by a catch(Exception). In some cases, we need to catch the Eroor too, and I rethrow it.

        Show
        elecharny Emmanuel Lecharny added a comment - I removed all the places where we were doing a catch (Throwable) and replaced it by a catch(Exception). In some cases, we need to catch the Eroor too, and I rethrow it.
        Hide
        janicki Chris Janicki added a comment -

        Emmanuel, thanks for the quick and intelligent response. Julien, thanks for referencing the java.lang.Error javadoc… You did a better job making my point.

        In summary, I think the "best practice" is: always catch Exception, but never catch Error. (And since Throwable is the superclass of both, never catch that either.)

        There are always exceptions to rules, of course. So if you feel you must catch Throwable, then I appreciate Emmanuel's effort to catch it again in order to at least print something.

        Show
        janicki Chris Janicki added a comment - Emmanuel, thanks for the quick and intelligent response. Julien, thanks for referencing the java.lang.Error javadoc… You did a better job making my point. In summary, I think the "best practice" is: always catch Exception, but never catch Error. (And since Throwable is the superclass of both, never catch that either.) There are always exceptions to rules, of course. So if you feel you must catch Throwable, then I appreciate Emmanuel's effort to catch it again in order to at least print something.
        Hide
        vrm Julien Vermillard added a comment -

        Agreeing with Emmanuel except for that :

        } catch (Throwable t) { // <<----- Here we suppose we have a pb with the logger, and we simply dump a stackTrace
        

        please don't catch Throwable but Exception,
        Throwable not Exception are java.lang.Error : prettry serious errors like out-of-memory or classloading stuff, you have a good chance to not being able to log it.

        Extract of java.lang.Error javadoc :

        /**
         * An <code>Error</code> is a subclass of <code>Throwable</code> 
         * that indicates serious problems that a reasonable application 
         * should not try to catch. Most such errors are abnormal conditions. 
         * The <code>ThreadDeath</code> error, though a "normal" condition,
         * is also a subclass of <code>Error</code> because most applications
         * should not try to catch it. 
         * <p>
        
        Show
        vrm Julien Vermillard added a comment - Agreeing with Emmanuel except for that : } catch (Throwable t) { // <<----- Here we suppose we have a pb with the logger, and we simply dump a stackTrace please don't catch Throwable but Exception, Throwable not Exception are java.lang.Error : prettry serious errors like out-of-memory or classloading stuff, you have a good chance to not being able to log it. Extract of java.lang.Error javadoc : /** * An <code>Error</code> is a subclass of <code>Throwable</code> * that indicates serious problems that a reasonable application * should not try to catch . Most such errors are abnormal conditions. * The <code>ThreadDeath</code> error, though a "normal" condition, * is also a subclass of <code>Error</code> because most applications * should not try to catch it. * <p>
        Hide
        elecharny Emmanuel Lecharny added a comment - - edited

        You are perfectly right, we should not catch exceptions.

        Except that in MINA, when an exception occurs, we catch it to generate a ExceptionCaught event for the Handler to manage it the way it wants. So far, so good : the IoHandler always gets the required informations needed to manage the exception.

        Except that when the exception is generated by a bad log library, we can't rethrow the exception, otherwise you'll enter into an infinite loop, as the exception will be caught, generate a ExceptionCaught event, which will throw an excption, etc...

        At this point, I don't know how to avoid such a rare scenario... May be something like :

            private void callNextExceptionCaught(Entry entry, IoSession session, Throwable cause) {
                ...
                    try {
                        IoFilter filter = entry.getFilter();
                        NextFilter nextFilter = entry.getNextFilter();
                        filter.exceptionCaught(nextFilter, session, cause);
                    } catch (Throwable e) {
                        try {
                            LOGGER.warn("Unexpected exception from exceptionCaught handler.", e);
                        } catch (Throwable t) {       // <<----- Here we suppose we have a pb with the logger, and we simply dump a stackTrace
                            t.printStackTrace();
                        }
                    }
        

        If you have any better idea, feel free to propose something that could be helpful.

        Show
        elecharny Emmanuel Lecharny added a comment - - edited You are perfectly right, we should not catch exceptions. Except that in MINA, when an exception occurs, we catch it to generate a ExceptionCaught event for the Handler to manage it the way it wants. So far, so good : the IoHandler always gets the required informations needed to manage the exception. Except that when the exception is generated by a bad log library, we can't rethrow the exception, otherwise you'll enter into an infinite loop, as the exception will be caught, generate a ExceptionCaught event, which will throw an excption, etc... At this point, I don't know how to avoid such a rare scenario... May be something like : private void callNextExceptionCaught(Entry entry, IoSession session, Throwable cause) { ... try { IoFilter filter = entry.getFilter(); NextFilter nextFilter = entry.getNextFilter(); filter.exceptionCaught(nextFilter, session, cause); } catch (Throwable e) { try { LOGGER.warn( "Unexpected exception from exceptionCaught handler." , e); } catch (Throwable t) { // <<----- Here we suppose we have a pb with the logger, and we simply dump a stackTrace t.printStackTrace(); } } If you have any better idea, feel free to propose something that could be helpful.

          People

          • Assignee:
            Unassigned
            Reporter:
            janicki Chris Janicki
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development