Qpid
  1. Qpid
  2. QPID-1670

Errors thrown from onMessage can kill the dispatcher

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.5, 0.6, 0.7, 0.8
    • Fix Version/s: 0.9
    • Component/s: Java Client
    • Labels:
      None

      Description

      BasicMessageConsumer.notifyMessage catches Exception and handles it, but unchecked throwables will cause the Dispatcher thread in the Session to terminate after logging. It does not kill the session.

      The JMS spec has this to say:
      4.5.2 Asynchronous Delivery
      A client can register an object that implements the JMS MessageListener
      interface with a MessageConsumer. As messages arrive for the consumer, the
      provider delivers them by calling the listener's onMessage method.
      It is possible for a listener to throw a RuntimeException; however, this is
      considered a client programming error. Well-behaved listeners should catch
      such exceptions and attempt to divert messages causing them to some form of
      application-specific 'unprocessable message' destination.
      The result of a listener throwing a RuntimeException depends on the session's
      acknowledgment mode.
      • AUTO_ACKNOWLEDGE or DUPS_OK_ACKNOWLEDGE - the message
      will be immediately redelivered. The number of times a JMS provider will
      redeliver the same message before giving up is provider-dependent. The
      JMSRedelivered message header field will be set for a message redelivered
      under these circumstances.
      • CLIENT_ACKNOWLEDGE - the next message for the listener is delivered.
      If a client wishes to have the previous unacknowledged message
      redelivered, it must manually recover the session.
      • Transacted Session - the next message for the listener is delivered. The client
      can either commit or roll back the session (in other words, a
      RuntimeException does not automatically rollback the session).
      JMS providers should flag clients with message listeners that are throwing
      RuntimeExceptions as possibly malfunctioning.

        Activity

        Hide
        Aidan Skinner added a comment -

        I was about to fix this, but we do actually handle RuntimeException properly. What we don't handle are things that don't extend Exception, such as Error. I'm not sure that we want to handle those this way though, death may be a more appropriate response.

        Show
        Aidan Skinner added a comment - I was about to fix this, but we do actually handle RuntimeException properly. What we don't handle are things that don't extend Exception, such as Error. I'm not sure that we want to handle those this way though, death may be a more appropriate response.
        Hide
        Robbie Gemmell added a comment -

        Downgrading severity from Blocker in light of the previous comment and JIRA name change.

        Show
        Robbie Gemmell added a comment - Downgrading severity from Blocker in light of the previous comment and JIRA name change.
        Hide
        Keith Wall added a comment -

        It seems to me that it is reasonable for java.lang.Errors thrown within onMessage to cause the Dispatcher thread to die. Errors represent abnormal JVM conditions and the Javadoc for java.lang.Error specifically advises against trying to handle these error.

        However, I think it would improve the client if it were changed to log the exception to the application log (i.e. log via SLF4J) rather than relying on the logging to stderr provided as default behaviour from the JVM's ThreadGroup.uncaughtException. This would allow any log scraping employed by the user to detect the condition and respond accordingly (generate an alert, or restart the process)..

        Attaching patch that implements/registers an UncaughtExceptionHandler which logs uncaught exceptions that abruptly terminate threads to the application log (via SLF4J) with supporting system and unit tests

        Show
        Keith Wall added a comment - It seems to me that it is reasonable for java.lang.Errors thrown within onMessage to cause the Dispatcher thread to die. Errors represent abnormal JVM conditions and the Javadoc for java.lang.Error specifically advises against trying to handle these error. However, I think it would improve the client if it were changed to log the exception to the application log (i.e. log via SLF4J) rather than relying on the logging to stderr provided as default behaviour from the JVM's ThreadGroup.uncaughtException. This would allow any log scraping employed by the user to detect the condition and respond accordingly (generate an alert, or restart the process).. Attaching patch that implements/registers an UncaughtExceptionHandler which logs uncaught exceptions that abruptly terminate threads to the application log (via SLF4J) with supporting system and unit tests
        Hide
        Keith Wall added a comment -

        Patch to implement UncaughtExceptionHandler to write errors causing abrupt termination of threads to (client) application log.

        Show
        Keith Wall added a comment - Patch to implement UncaughtExceptionHandler to write errors causing abrupt termination of threads to (client) application log.
        Hide
        Robbie Gemmell added a comment -

        Patch applied.

        Show
        Robbie Gemmell added a comment - Patch applied.

          People

          • Assignee:
            Robbie Gemmell
            Reporter:
            Aidan Skinner
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development