[jira] Commented: (QPID-2337) SubscriptionManager::start() does not handle exceptions from dispatch

["Tom McCubbin (JIRA)" <qp...@incubator.apache.org>]

    [ https://issues.apache.org/jira/browse/QPID-2337?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12845672#action_12845672 ] 

Tom McCubbin commented on QPID-2337:
------------------------------------

After spending a few hours on this (yeah...should have been a bit quicker...) I came to the same conclusion. I was happy to find the SubscriptionManager::start() method that starts a new thread from whence msgs can be dispatched. Until I found out that start() which is handled via the qpid::client::Dispatcher just fires a thread which calls qpid::client::Dispatcher::run(). Run in turn may throw a ClosedException, TransportFailure, or std::exception.

When in a separate thread, any exceptions are unhandled and result in the untimely demise of that thread and every other thread, unless I make wild presumptions in my unhandled exception handler, which is not realistic.

Furthermore, ClosedException and TransportFailure do not call the failoverHandler, yet std::exceptions do? This seems quite awkward. If you start w/out a broker available, it reliably calls the failoverHandler(). If however you crash the server, it throws and you are done.

As any old code-monkey does, I assume at some point the connected server will crash. Not if, but when. This behaviour is not acceptable.

To fix it, I did two very simple things. Both change qpidc-0.5/src/qpid/client/Dispatcher.cpp (no header change)

1) I changed the qpid::client::Dispatcher::run() method so if there is an exception, ANY TYPE, and if there is a failoverHandler registered that handler will be called, and no exception will be thrown

Added below: (+++ and the 'e' for the first 2 catch clauses )

    catch (const ClosedException& e) {
+++ if ( failoverHandler ) {
+++ QPID_LOG(debug, QPID_MSG(session.getId() << " failover: " << e.what()));
+++ failoverHandler();
+++ } else {
QPID_LOG(debug, QPID_MSG(session.getId() << ": closed by peer"));
+++ }
    }
    catch (const TransportFailure& e) {
+++ if ( failoverHandler ) {
+++ QPID_LOG(debug, QPID_MSG(session.getId() << " failover: " << e.what()));
+++ failoverHandler();
+++ } else {
QPID_LOG(info, QPID_MSG(session.getId() << ": transport failure"));
throw;
+++ }
    }
    catch (const std::exception& e) {
        if ( failoverHandler ) {
            QPID_LOG(debug, QPID_MSG(session.getId() << " failover: " << e.what()));
            failoverHandler();
        } else {
            QPID_LOG(error, session.getId() << " error: " << e.what());
            throw;
        }
    }

2) when the qpid::client::Dispatcher::start() method is called, if there is no failoverHandler registered, I register an anonymous dummy function I added to the same .cpp file. Looks like this.

+++namespace {
+++ void dumbHandler() { }
+++}

void Dispatcher::start()
{
+++ if ( ! failoverHandler )
+++ registerFailoverHandler( dumbHandler );

    worker = Thread(this);
}

This is working wonderfully for me so far. Yeah, its not perfect, but hey its quicker than adding my own thread which can have the try/catch code to wrap the run() method.

In practice, if you register a failureCallback on the qpid::client::Connection to handle these errors you need not worry about any other handler for SubscriptionManager(s) and the code above will bring you quickly to Shangri La.

If you want, you can still register a failureCallback on the SubscriptionManager and provided you use my modified run() method, you will be ok as well.

If anyone wants a patch, let me know. 

> SubscriptionManager::start() does not handle exceptions from dispatch
> ---------------------------------------------------------------------
>
>                 Key: QPID-2337
>                 URL: https://issues.apache.org/jira/browse/QPID-2337
>             Project: Qpid
>          Issue Type: Bug
>          Components: C++ Client
>    Affects Versions: 0.5, 0.6
>            Reporter: Gordon Sim
>            Assignee: Gordon Sim
>             Fix For: 0.7
>
>
> ...so any exceptions thrown (e.g. when connection is lost) will cause client process to terminate if start() is used.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project:      http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org