[asterisk-scf-commits] asterisk-scf/integration/sip.git branch "x_safe_constructors" updated.

Commits to the Asterisk SCF project code repositories asterisk-scf-commits at lists.digium.com
Tue Jul 17 11:18:37 CDT 2012


branch "x_safe_constructors" has been updated
       via  dfa8647bf2e31c8de9cc0a7617049b061ed8ed9f (commit)
      from  288cdf2f20572064244732b8a897342fb9c3792a (commit)

Summary of changes:
 src/PJSIPSessionModule.h |   16 ++
 src/SIPSession.cpp       |  425 +++++++++++++++++++++++++---------------------
 src/SIPSession.h         |   21 +++-
 3 files changed, 269 insertions(+), 193 deletions(-)


- Log -----------------------------------------------------------------
commit dfa8647bf2e31c8de9cc0a7617049b061ed8ed9f
Author: Brent Eagles <beagles at digium.com>
Date:   Tue Jul 17 13:40:13 2012 -0230

    A large number of small changes:
    
     - Operation contexts are checked before enqueuing AMD ops, thereby reducing
       the amount of thread creation-per-operation bashing that the thread pool is
       currently exhibiting. This is better even if the thread pool were working
       the way we want.
    
     - Added a small "hack-like" change to SIPAMICallback to remove references to
       the SIPSession, etc once it is done with them. The reason is that the callback
       object is actually held onto by AsyncResult. AsyncResult is being passed
       back to an enqueued operation, etc. I cannot see at the moment how that
       could be causing the problem we are seeing at the moment, but this change
       might help make the bug manifest itself closer to its cause.

diff --git a/src/PJSIPSessionModule.h b/src/PJSIPSessionModule.h
index d08b475..4e67cd1 100644
--- a/src/PJSIPSessionModule.h
+++ b/src/PJSIPSessionModule.h
@@ -373,6 +373,8 @@ public:
         {
             mSession->enqueueSessionWork(new SIPAMICallbackOperation(r, mOperation));
         }
+
+        clear();
     }
 
 private:
@@ -405,6 +407,20 @@ private:
      * and mIsSuspended should never both be true.
      */
     bool mNeedsRequeuing;
+
+    void clear() 
+    {
+        //
+        // kind of a hack to see if this is what is causing the problem. The
+        // ami AsyncResult holds a reference to the callback, which we pass to
+        // an operation. There is just a lot of weird final references when the
+        // stack unwinds. This might make it a bit easier to figure out what is
+        // going on.
+        //
+        mSession = 0;
+        mListener = 0;
+        mOperation = 0;
+    } 
 };
 
 typedef IceUtil::Handle<SIPAMICallback> SIPAMICallbackPtr;
diff --git a/src/SIPSession.cpp b/src/SIPSession.cpp
index e6ce9dc..1b326f9 100755
--- a/src/SIPSession.cpp
+++ b/src/SIPSession.cpp
@@ -704,24 +704,12 @@ class ChangeStreamStatesOperation : public SuspendableWork
 public:
     ChangeStreamStatesOperation(
         const AsteriskSCF::SessionCommunications::V1::AMD_SessionController_changeStreamStatesPtr& cb,
-        const OperationContextPtr& operationContext,
         const AsteriskSCF::Media::V1::StreamStateDict& streams,
         const boost::shared_ptr<SIPSessionPriv>& sessionPriv)
-        : mCb(cb), mOperationContext(operationContext), mStreams(streams), mImplPriv(sessionPriv) { }
+        : mCb(cb), mStreams(streams), mImplPriv(sessionPriv) { }
     
     SuspendableWorkResult execute(const SuspendableWorkListenerPtr&)
     {
-        AMDContextData<AMD_SessionController_changeStreamStatesPtr>::ptr_type operationCookie = 
-            getContext<AMDContextData<AMD_SessionController_changeStreamStatesPtr> >(
-                mImplPriv->mOperationContextCache,
-                mOperationContext,
-                mCb);
-        if (!operationCookie)
-        {
-            lg(Debug) << "changeStreamStates() detected retry for operation " << mOperationContext->id;
-            return Complete;
-        }
-
         lg(Debug) << "Executing a changeStreamStates Operation";
 
         try
@@ -818,11 +806,11 @@ public:
                 }
             }
 
-            operationCookie->setCompleted();
+            mCb->ice_response();
         }
         catch (const std::exception& e)
         {
-            operationCookie->setException(ExceptionWrapperPtr(new ExceptionWrapper(e)));
+            mCb->ice_exception(e);
             assert(false);
         }
         return Complete;
@@ -830,7 +818,6 @@ public:
     
 private:
     AsteriskSCF::SessionCommunications::V1::AMD_SessionController_changeStreamStatesPtr mCb;
-    OperationContextPtr mOperationContext;
     AsteriskSCF::Media::V1::StreamStateDict mStreams;
     boost::shared_ptr<SIPSessionPriv> mImplPriv;
 };
@@ -840,29 +827,22 @@ class AddStreamsOperation : public SuspendableWork
 public:
     AddStreamsOperation(
         const AsteriskSCF::SessionCommunications::V1::AMD_SessionController_addStreamsPtr& cb,
-        const OperationContextPtr& operationContext,
+        const OperationContextPtr& opContext,
         const AsteriskSCF::Media::V1::StreamInformationDict& streams,
         const boost::shared_ptr<SIPSessionPriv>& sessionPriv,
         const SIPSessionPtr& session)
-        : mCb(cb), mOperationContext(operationContext), mStreams(streams), mImplPriv(sessionPriv), mSession(session) { }
+        : mCb(cb), mOperationContext(opContext), mStreams(streams), mImplPriv(sessionPriv), mSession(session) { }
 
     SuspendableWorkResult execute(const SuspendableWorkListenerPtr&)
     {
-        AMDContextResultData<StreamInformationDict, AMD_SessionController_addStreamsPtr>::ptr_type operationCookie = 
-            getContext<AMDContextResultData<StreamInformationDict, AMD_SessionController_addStreamsPtr> >(
-                mImplPriv->mOperationContextCache,
-                mOperationContext,
-                mCb);
-        if (!operationCookie)
-        {
-            lg(Debug) << "addStreams() detected retry for operation " << mOperationContext->id;
-            return Complete;
-        }
-
         try
         {
             lg(Debug) << "Executing an addStreams Operation";
 
+            //
+            // XXX this is seriously screwy code!
+            //
+
             // If there is an outstanding transaction then no streams can be added at this time
             if (mImplPriv->mInviteSession->invite_tsx)
             {
@@ -883,7 +863,7 @@ public:
 
             // Store callback information so when the remote party responds with which streams were accepted we can
             // communicate it to the controller
-            mImplPriv->mAddStreamsCb = mCb;
+            mImplPriv->mAddStreamsCb = mCb; // XXX - this is wrong. And it should've been set before createSDPOffer() was called!
 
             // Okay, create and send the reinvite!
             pjsip_tx_data *packet = NULL;
@@ -897,12 +877,15 @@ public:
                 // If we couldn't create the reinvite no streams were added
                 lg(Warning) << "Unable to create reinvite when adding streams";
             }
-        
-            operationCookie->setResult(StreamInformationDict());
+       
+            //
+            // If the callback is supposed to happen in createSDPOffer, then this shouldn't be happening here!
+            //
+            mCb->ice_response(StreamInformationDict());
         }
         catch (const std::exception& e)
         {
-            operationCookie->setException(ExceptionWrapperPtr(new ExceptionWrapper(e)));
+            mCb->ice_exception(e);
             assert(false);
         }
         return Complete;
@@ -910,7 +893,7 @@ public:
 
 private:
     AsteriskSCF::SessionCommunications::V1::AMD_SessionController_addStreamsPtr mCb;
-    OperationContextPtr mOperationContext;
+    AsteriskSCF::System::V1::OperationContextPtr mOperationContext;
     AsteriskSCF::Media::V1::StreamInformationDict mStreams;
     boost::shared_ptr<SIPSessionPriv> mImplPriv;
     SIPSessionPtr mSession;
@@ -921,25 +904,13 @@ class RemoveStreamsOperation : public SuspendableWork
 public:
     RemoveStreamsOperation(
         const AsteriskSCF::SessionCommunications::V1::AMD_SessionController_removeStreamsPtr& cb,
-        const OperationContextPtr& operationContext,
         const AsteriskSCF::Media::V1::StreamInformationDict& streams,
         const boost::shared_ptr<SIPSessionPriv>& sessionPriv,
         const SIPSessionPtr& session)
-        : mCb(cb), mOperationContext(operationContext), mStreams(streams), mImplPriv(sessionPriv), mSession(session) { }
+        : mCb(cb), mStreams(streams), mImplPriv(sessionPriv), mSession(session) { }
 
     SuspendableWorkResult execute(const SuspendableWorkListenerPtr&)
     {
-        AMDContextData<AMD_SessionController_removeStreamsPtr>::ptr_type operationCookie = 
-            getContext<AMDContextData<AMD_SessionController_removeStreamsPtr> >(
-                mImplPriv->mOperationContextCache,
-                mOperationContext,
-                mCb);
-        if (!operationCookie)
-        {
-            lg(Debug) << "removeStreamStates() detected retry for operation " << mOperationContext->id;
-            return Complete;
-        }
-
         try
         {
             lg(Debug) << "Executing a removeStreams Operation";
@@ -964,13 +935,16 @@ public:
             {
                 pjsip_inv_set_sdp_answer(mImplPriv->mInviteSession, sdp);
             }
-
-            operationCookie->setCompleted();
+            mCb->ice_response();
         }
         catch (const std::exception& e)
         {
-            operationCookie->setException(ExceptionWrapperPtr(new ExceptionWrapper(e)));
+            mCb->ice_exception(e);
+        }
+        catch (...)
+        {
             assert(false);
+            mCb->ice_exception();
         }
 
         return Complete;
@@ -978,7 +952,6 @@ public:
 
 private:
     AsteriskSCF::SessionCommunications::V1::AMD_SessionController_removeStreamsPtr mCb;
-    OperationContextPtr mOperationContext;
     AsteriskSCF::Media::V1::StreamInformationDict mStreams;
     boost::shared_ptr<SIPSessionPriv> mImplPriv;
     SIPSessionPtr mSession;
@@ -1040,31 +1013,67 @@ public:
                                   const OperationContextPtr& operationContext,
                                   const AsteriskSCF::Media::V1::StreamStateDict& states, const Ice::Current&)
     {
+        AMDContextData<AMD_SessionController_changeStreamStatesPtr>::ptr_type operationCookie = 
+            getContext<AMDContextData<AMD_SessionController_changeStreamStatesPtr> >(
+                mImplPriv->mOperationContextCache,
+                operationContext,
+                cb);
+        if (!operationCookie)
+        {
+            lg(Debug) << "changeStreamStates() detected retry for operation " << operationContext->id;
+            return;
+        }
         lg(Debug) << "Queueing changeStreamStates operation";
-        mSession->enqueueSessionWork(new ChangeStreamStatesOperation(cb, operationContext, states, mImplPriv));
+        mSession->enqueueSessionWork(new ChangeStreamStatesOperation(operationCookie->getProxy(),
+                states, mImplPriv));
     }
     
     void addStreams_async(const AsteriskSCF::SessionCommunications::V1::AMD_SessionController_addStreamsPtr& cb,
                           const OperationContextPtr& operationContext,
                           const AsteriskSCF::Media::V1::StreamInformationDict& streams, const Ice::Current&)
     {
+        AMDContextResultData<StreamInformationDict, AMD_SessionController_addStreamsPtr>::ptr_type operationCookie = 
+            getContext<AMDContextResultData<StreamInformationDict, AMD_SessionController_addStreamsPtr> >(
+                mImplPriv->mOperationContextCache,
+                operationContext,
+                cb);
+        if (!operationCookie)
+        {
+            lg(Debug) << "addStreams() detected retry for operation " << operationContext->id;
+            return;
+        }
+
         lg(Debug) << "Queueing addStreams operation";
-        mSession->enqueueSessionWork(new AddStreamsOperation(cb, operationContext, streams, mImplPriv, mSession));
+        mSession->enqueueSessionWork(new AddStreamsOperation(operationCookie->getProxy(), operationContext,
+                streams, mImplPriv, mSession));
     }
     
     void removeStreams_async(const AsteriskSCF::SessionCommunications::V1::AMD_SessionController_removeStreamsPtr& cb,
                              const OperationContextPtr& operationContext,
                              const AsteriskSCF::Media::V1::StreamInformationDict& streams, const Ice::Current&)
     {
+        AMDContextData<AMD_SessionController_removeStreamsPtr>::ptr_type operationCookie = 
+            getContext<AMDContextData<AMD_SessionController_removeStreamsPtr> >(
+                mImplPriv->mOperationContextCache,
+                operationContext,
+                cb);
+        if (!operationCookie)
+        {
+            lg(Debug) << "removeStreamStates() detected retry for operation " << operationContext->id;
+            return;
+        }
+
         lg(Debug) << "Queueing removeStreams operation";
-        mSession->enqueueSessionWork(new RemoveStreamsOperation(cb, operationContext, streams, mImplPriv, mSession));
+        mSession->enqueueSessionWork(new RemoveStreamsOperation(operationCookie->getProxy(),
+                streams, mImplPriv, mSession));
     }
     
     /**
      * This operation allows the externally connected component (typically the bridge)
      * to update this session's ConnectedLine information. 
      */
-    void updateConnectedLine(const OperationContextPtr& operationContext, const ConnectedLinePtr& connected, const Ice::Current&) 
+    void updateConnectedLine(const OperationContextPtr& operationContext, const ConnectedLinePtr& connected,
+        const Ice::Current&) 
     {
         ContextDataPtr operationCookie;
         if (!(operationCookie = Operations::checkAndThrow(mOperationContextCache, operationContext)))
@@ -1199,25 +1208,13 @@ class ConnectStreamsOperation : public SuspendableWork
 public:
     ConnectStreamsOperation(
         const AsteriskSCF::Media::V1::AMD_DirectMediaConnection_connectStreamsPtr& cb,
-        const OperationContextPtr& operationContext,
         const AsteriskSCF::Media::V1::DirectMediaConnectionDict& connections,
         const boost::shared_ptr<SIPSessionPriv>& implPriv,
         const SIPSessionPtr& session)
-        : mCb(cb), mOperationContext(operationContext), mConnections(connections), mImplPriv(implPriv), mSession(session) { }
+        : mCb(cb), mConnections(connections), mImplPriv(implPriv), mSession(session) { }
 
     SuspendableWorkResult execute(const SuspendableWorkListenerPtr&)
     {
-        AMDContextData<AMD_DirectMediaConnection_connectStreamsPtr>::ptr_type operationCookie = 
-            getContext<AMDContextData<AMD_DirectMediaConnection_connectStreamsPtr> >(
-                mImplPriv->mOperationContextCache,
-                mOperationContext,
-                mCb);
-        if (!operationCookie)
-        {
-            lg(Debug) << "connectStreams() detected retry for operation " << mOperationContext->id;
-            return Complete;
-        }
-
         try
         {
             if (mSession->getInviteSession()->invite_tsx)
@@ -1238,12 +1235,16 @@ public:
                 lg(Warning) << "Unable to create reinvite to connect streams";
             }
 
-            operationCookie->setCompleted();
+            mCb->ice_response();
         }
         catch (const std::exception& e)
         {
-            operationCookie->setException(ExceptionWrapperPtr(new ExceptionWrapper(e)));
+            mCb->ice_exception(e);
+        }
+        catch (...)
+        {
             assert(false);
+            mCb->ice_exception();
         }
 
         return Complete;
@@ -1251,7 +1252,6 @@ public:
 
 private:
     AsteriskSCF::Media::V1::AMD_DirectMediaConnection_connectStreamsPtr mCb;
-    OperationContextPtr mOperationContext;
     AsteriskSCF::Media::V1::DirectMediaConnectionDict mConnections;
     boost::shared_ptr<SIPSessionPriv> mImplPriv;
     const SIPSessionPtr mSession;
@@ -1262,25 +1262,13 @@ class DisconnectStreamsOperation : public SuspendableWork
 public:
     DisconnectStreamsOperation(
         const AsteriskSCF::Media::V1::AMD_DirectMediaConnection_disconnectStreamsPtr& cb,
-        const OperationContextPtr& operationContext,
         const Ice::StringSeq& streams,
         const boost::shared_ptr<SIPSessionPriv>& implPriv,
         const SIPSessionPtr& session)
-        : mCb(cb), mOperationContext(operationContext), mStreams(streams), mImplPriv(implPriv), mSession(session) { }
+        : mCb(cb), mStreams(streams), mImplPriv(implPriv), mSession(session) { }
 
     SuspendableWorkResult execute(const SuspendableWorkListenerPtr&)
     {
-        AMDContextData<AMD_DirectMediaConnection_disconnectStreamsPtr>::ptr_type operationCookie = 
-            getContext<AMDContextData<AMD_DirectMediaConnection_disconnectStreamsPtr> >(
-                mImplPriv->mOperationContextCache,
-                mOperationContext,
-                mCb);
-        if (!operationCookie)
-        {
-            lg(Debug) << "removeStreamStates() detected retry for operation " << mOperationContext->id;
-            return Complete;
-        }
-
         try
         {
             pjmedia_sdp_session *sdp = mSession->modifySDP(mStreams);
@@ -1295,12 +1283,16 @@ public:
                 lg(Warning) << "Unable to create reinvite to disconnect streams";
             }
 
-            operationCookie->setCompleted();
+            mCb->ice_response();
         }
         catch (const std::exception& e)
         {
-            operationCookie->setException(ExceptionWrapperPtr(new ExceptionWrapper(e)));
+            mCb->ice_exception(e);
+        }
+        catch (...)
+        {
             assert(false);
+            mCb->ice_exception();
         }
 
         return Complete;
@@ -1308,7 +1300,6 @@ public:
 
 private:
     AsteriskSCF::Media::V1::AMD_DirectMediaConnection_disconnectStreamsPtr mCb;
-    OperationContextPtr mOperationContext;
     Ice::StringSeq mStreams;
     boost::shared_ptr<SIPSessionPriv> mImplPriv;
     const SIPSessionPtr mSession;
@@ -1334,16 +1325,39 @@ public:
                               const AsteriskSCF::System::V1::OperationContextPtr& operationContext,
                               const AsteriskSCF::Media::V1::DirectMediaConnectionDict& connections, const Ice::Current&)
     {
+        AMDContextData<AMD_DirectMediaConnection_connectStreamsPtr>::ptr_type operationCookie = 
+            getContext<AMDContextData<AMD_DirectMediaConnection_connectStreamsPtr> >(
+                mImplPriv->mOperationContextCache,
+                operationContext,
+                cb);
+        if (!operationCookie)
+        {
+            lg(Debug) << "connectStreams() detected retry for operation " << operationContext->id;
+            return;
+        }
         lg(Debug) << "Queueing connectStreams operation";
-        mSession->enqueueSessionWork(new ConnectStreamsOperation(cb, operationContext, connections, mImplPriv, mSession));
+        mSession->enqueueSessionWork(new ConnectStreamsOperation(operationCookie->getProxy(),
+                connections, mImplPriv, mSession));
     }
 
     void disconnectStreams_async(const AsteriskSCF::Media::V1::AMD_DirectMediaConnection_disconnectStreamsPtr& cb,
                                  const AsteriskSCF::System::V1::OperationContextPtr& operationContext,
                                  const Ice::StringSeq& streams, const Ice::Current&)
     {
+        AMDContextData<AMD_DirectMediaConnection_disconnectStreamsPtr>::ptr_type operationCookie = 
+            getContext<AMDContextData<AMD_DirectMediaConnection_disconnectStreamsPtr> >(
+                mImplPriv->mOperationContextCache,
+                operationContext,
+                cb);
+        if (!operationCookie)
+        {
+            lg(Debug) << "removeStreamStates() detected retry for operation " << operationContext->id;
+            return;
+        }
+
         lg(Debug) << "Queueing disconnectStreams operation";
-        mSession->enqueueSessionWork(new DisconnectStreamsOperation(cb, operationContext, streams, mImplPriv, mSession));
+        mSession->enqueueSessionWork(new DisconnectStreamsOperation(operationCookie->getProxy(),
+                streams, mImplPriv, mSession));
     }
 
 private:
@@ -1600,6 +1614,10 @@ SIPSessionPtr SIPSession::create(
         hooks.push_back(oneShotHook);
     }
     newSession->activateIceObjects(hooks, config, isUAC);
+    //
+    // TODO: introduce try/catch block to destroy the newSession object and
+    // cleanup servants on exceptions during initialization.
+    //
 
     return newSession;
 }
@@ -1652,6 +1670,15 @@ SIPSessionPtr SIPSession::create(
         hooks.push_back(oneShotHook);
     }
     newSession->activateIceObjects(hooks, config, isUAC);
+    //
+    // TODO: introduce try/catch block to destroy the newSession object and
+    // cleanup servants on exceptions during initialization.  Also, a
+    // replica that fails to initialize standby objects is in a
+    // questionable state, so it may be desirable to bounce the replica. On
+    // the other hand, we could simply add the notion that replication is
+    // best-effort and that a certain subset of replication failures is
+    // allowable.
+    //
 
     return newSession;
 }
@@ -1724,35 +1751,27 @@ class AddListenerOperation : public SuspendableWork
 public:
     AddListenerOperation(
             const AsteriskSCF::SessionCommunications::V1::AMD_Session_addListenerPtr& cb,
-            const OperationContextPtr& operationContext,
             const AsteriskSCF::SessionCommunications::V1::SessionListenerPrx& listener,
             const boost::shared_ptr<SIPSessionPriv>& implPriv,
             const SIPSessionPtr& session)
-        : mCb(cb), mOperationContext(operationContext), mListener(listener), mImplPriv(implPriv), mSession(session) { }
+        : mCb(cb), mListener(listener), mImplPriv(implPriv), mSession(session) { }
 
     SuspendableWorkResult execute(const SuspendableWorkListenerPtr&)
     {
-        AMDContextResultData<SessionInfoPtr, AMD_Session_addListenerPtr>::ptr_type operationCookie = 
-            getContext<AMDContextResultData<SessionInfoPtr, AMD_Session_addListenerPtr> >(
-                mImplPriv->mOperationContextCache,
-                mOperationContext,
-                mCb);
-        if (!operationCookie)
-        {
-            lg(Debug) << "addListener() detected retry for operation " << mOperationContext->id;
-            return Complete;
-        }
-
         try
         {
             lg(Debug) << "Executing addListener operation";
             mSession->addListener(mListener);
-            operationCookie->setResult(mSession->getInfo());
+            mCb->ice_response(mSession->getInfo());
         }
         catch (const std::exception& e)
         {
-            operationCookie->setException(ExceptionWrapperPtr(new ExceptionWrapper(e)));
+            mCb->ice_exception(e);
+        }
+        catch (...)
+        {
             assert(false);
+            mCb->ice_exception();
         }
 
         return Complete;
@@ -1771,8 +1790,18 @@ void SIPSession::addListener_async(
         const AsteriskSCF::SessionCommunications::V1::SessionListenerPrx& listener,
         const Ice::Current&)
 {
+    AMDContextResultData<SessionInfoPtr, AMD_Session_addListenerPtr>::ptr_type operationCookie = 
+        getContext<AMDContextResultData<SessionInfoPtr, AMD_Session_addListenerPtr> >(
+            mImplPriv->mOperationContextCache,
+            operationContext,
+            cb);
+    if (!operationCookie)
+    {
+        lg(Debug) << "addListener() detected retry for operation " << operationContext->id;
+        return;
+    }
     lg(Debug) << "Queueing addListener operation";
-    enqueueSessionWork(new AddListenerOperation(cb, operationContext, listener, mImplPriv, this));
+    enqueueSessionWork(new AddListenerOperation(operationCookie->getProxy(), listener, mImplPriv, this));
 }
 
 void SIPSession::addListener(const AsteriskSCF::SessionCommunications::V1::SessionListenerPrx& listener)
@@ -1796,25 +1825,13 @@ class IndicateOperation : public SuspendableWork
 public:
     IndicateOperation(
             const AsteriskSCF::SessionCommunications::V1::AMD_Session_indicatePtr& cb,
-            const AsteriskSCF::System::V1::OperationContextPtr& operationContext,
             const AsteriskSCF::SessionCommunications::V1::IndicationPtr& indication,
             const boost::shared_ptr<SIPSessionPriv>& sessionPriv,
             const SIPSessionPtr& session)
-        : mCb(cb), mOperationContext(operationContext), mIndication(indication), mImplPriv(sessionPriv), mSession(session) { }
+        : mCb(cb), mIndication(indication), mImplPriv(sessionPriv), mSession(session) { }
 
     SuspendableWorkResult execute(const SuspendableWorkListenerPtr&)
     {
-        AMDContextData<AMD_Session_indicatePtr>::ptr_type operationCookie = 
-            getContext<AMDContextData<AMD_Session_indicatePtr> >(
-                mImplPriv->mOperationContextCache,
-                mOperationContext,
-                mCb);
-        if (!operationCookie)
-        {
-            lg(Debug) << "indicate() detected retry for operation " << mOperationContext->id;
-            return Complete;
-        }
-
         try
         {
             lg(Debug) << "Executing indicate operation";
@@ -1872,12 +1889,16 @@ public:
                 pjsip_inv_send_msg(mImplPriv->mInviteSession, packet);
             }
 
-            operationCookie->setCompleted();
+            mCb->ice_response();
         }
         catch (const std::exception& e)
         {
-            operationCookie->setException(ExceptionWrapperPtr(new ExceptionWrapper(e)));
+            mCb->ice_exception(e);
+        }
+        catch (...)
+        {
             assert(false);
+            mCb->ice_exception();
         }
 
         return Complete;
@@ -1915,6 +1936,9 @@ private:
                 connectedName = connectedNumber;
             }
             pj_str_t paiValue;
+            //
+            // XXX: I strongly distrust this method of accessing something that can be modified in another thread!
+            //
             SIPEndpointConfig &config = mSession->getEndpoint()->getConfig();
             std::string ss = "\"" + connectedName + "\" <" + connectedNumber + "@" + config.sessionConfig.sourceAddress + ">;party=called";
             pj_strset(&paiValue, (char *) ss.c_str(), ss.length());
@@ -1954,8 +1978,19 @@ void SIPSession::indicate_async(
         const AsteriskSCF::SessionCommunications::V1::IndicationPtr& indication,
         const Ice::Current&)
 {
+    AMDContextData<AMD_Session_indicatePtr>::ptr_type operationCookie = 
+        getContext<AMDContextData<AMD_Session_indicatePtr> >(
+            mImplPriv->mOperationContextCache,
+            operationContext,
+            cb);
+    if (!operationCookie)
+    {
+        lg(Debug) << "indicate() detected retry for operation " << operationContext->id;
+        return;
+    }
+
     lg(Debug) << "Queuing an indicate operation";
-    enqueueSessionWork(new IndicateOperation(cb, operationContext, indication, mImplPriv, this));
+    enqueueSessionWork(new IndicateOperation(operationCookie->getProxy(), indication, mImplPriv, this));
 }
 
 class GetEndpointOperation : public SuspendableWork
@@ -1985,6 +2020,11 @@ void SIPSession::getEndpoint_async(
         const AsteriskSCF::SessionCommunications::V1::AMD_Session_getEndpointPtr& cb,
         const Ice::Current&)
 {
+    //
+    // TODO: AFAICT, the endpoint proxy for a session is immutable, in which
+    // case there is no real need to queue this work, we should be able to
+    // return immediately.
+    //
     lg(Debug) << "Queuing a getEndpoint operation";
     enqueueSessionWork(new GetEndpointOperation(cb, mImplPriv));
 }
@@ -2176,22 +2216,11 @@ public:
 
     SuspendableWorkResult execute(const SuspendableWorkListenerPtr&)
     {
-        AMDContextResultData<SessionControllerPrx, AMD_Session_setAndGetSessionControllerPtr>::ptr_type operationCookie = 
-            getContext<AMDContextResultData<SessionControllerPrx, AMD_Session_setAndGetSessionControllerPtr> >(
-                mImplPriv->mOperationContextCache,
-                mOperationContext,
-                mCb);
-        if (!operationCookie)
-        {
-            lg(Debug) << "getAndSetSessionController() detected retry for operation " << mOperationContext->id;
-            return Complete;
-        }
-
         lg(Debug) << "Executing a SetAndGetSessionController operation";
             
         if (mImplPriv->mSessionController)
         {
-            operationCookie->setException(AsteriskSCF::SessionCommunications::V1::ControllerAlreadySet());
+            mCb->ice_exception(AsteriskSCF::SessionCommunications::V1::ControllerAlreadySet());
             return Complete;
         }
 
@@ -2217,12 +2246,16 @@ public:
                 lg(Info) << "Unable to set ConnectedLine party info. No Id configured for endpoint " << mImplPriv->mEndpoint->getName(); 
             }
 
-            operationCookie->setResult(mImplPriv->mOurSessionControllerProxy);
+            mCb->ice_response(mImplPriv->mOurSessionControllerProxy);
         }
         catch (const std::exception& e)
         {
-            operationCookie->setException(ExceptionWrapperPtr(new ExceptionWrapper(e)));
+            mCb->ice_exception(e);
+        }
+        catch (...)
+        {
             assert(false);
+            mCb->ice_exception();
         }
 
         return Complete;
@@ -2244,8 +2277,18 @@ void SIPSession::setAndGetSessionController_async(
     const AsteriskSCF::SessionCommunications::V1::SessionControllerPrx& controller,
     const Ice::Current&)
 {
+    AMDContextResultData<SessionControllerPrx, AMD_Session_setAndGetSessionControllerPtr>::ptr_type operationCookie = 
+        getContext<AMDContextResultData<SessionControllerPrx, AMD_Session_setAndGetSessionControllerPtr> >(
+            mImplPriv->mOperationContextCache,
+            operationContext,
+            cb);
+    if (!operationCookie)
+    {
+        lg(Debug) << "getAndSetSessionController() detected retry for operation " << operationContext->id;
+        return;
+    }
     lg(Debug) << "queueing a setAndGetSessionController operation";
-    enqueueSessionWork(new SetAndGetSessionControllerOperation(cb, operationContext, controller, mImplPriv));
+    enqueueSessionWork(new SetAndGetSessionControllerOperation(operationCookie->getProxy(), operationContext, controller, mImplPriv));
 }
 
 class RemoveSessionControllerOperation : public SuspendableWork
@@ -2253,24 +2296,12 @@ class RemoveSessionControllerOperation : public SuspendableWork
 public:
     RemoveSessionControllerOperation(
         const AsteriskSCF::SessionCommunications::V1::AMD_Session_removeSessionControllerPtr& cb,
-        const OperationContextPtr& operationContext,
         const AsteriskSCF::SessionCommunications::V1::SessionControllerPrx& controller,
         const boost::shared_ptr<SIPSessionPriv>& sessionPriv)
-        : mCb(cb), mOperationContext(operationContext), mController(controller), mImplPriv(sessionPriv) { }
+        : mCb(cb), mController(controller), mImplPriv(sessionPriv) { }
 
     SuspendableWorkResult execute(const SuspendableWorkListenerPtr&)
     {
-        AMDContextData<AMD_Session_removeSessionControllerPtr>::ptr_type operationCookie = 
-            getContext<AMDContextData<AMD_Session_removeSessionControllerPtr> >(
-                mImplPriv->mOperationContextCache,
-                mOperationContext,
-                mCb);
-        if (!operationCookie)
-        {
-            lg(Debug) << "removeSessionController() detected retry for operation " << mOperationContext->id;
-            return Complete;
-        }
-
         lg(Debug) << "Executing a RemoveSessionController operation";
 
         if (mImplPriv->mSessionController == mController)
@@ -2278,13 +2309,12 @@ public:
             mImplPriv->mSessionController = 0;
         }
 
-        operationCookie->setCompleted();
+        mCb->ice_response();
         return Complete;
     }
 
 private:
     AsteriskSCF::SessionCommunications::V1::AMD_Session_removeSessionControllerPtr mCb;
-    OperationContextPtr mOperationContext;
     AsteriskSCF::SessionCommunications::V1::SessionControllerPrx mController;
     boost::shared_ptr<SIPSessionPriv> mImplPriv;
 };
@@ -2298,8 +2328,18 @@ void SIPSession::removeSessionController_async(
     const AsteriskSCF::SessionCommunications::V1::SessionControllerPrx& controller,
     const Ice::Current&)
 {
+    AMDContextData<AMD_Session_removeSessionControllerPtr>::ptr_type operationCookie = 
+        getContext<AMDContextData<AMD_Session_removeSessionControllerPtr> >(
+            mImplPriv->mOperationContextCache,
+            operationContext,
+            cb);
+    if (!operationCookie)
+    {
+        lg(Debug) << "removeSessionController() detected retry for operation " << operationContext->id;
+        return;
+    }
     lg(Debug) << "queueing a removeSessionController operation";
-    enqueueSessionWork(new RemoveSessionControllerOperation(cb, operationContext, controller, mImplPriv));
+    enqueueSessionWork(new RemoveSessionControllerOperation(operationCookie->getProxy(), controller, mImplPriv));
 }
 
 class SetBridgeOperation : public SuspendableWork
@@ -2307,41 +2347,33 @@ class SetBridgeOperation : public SuspendableWork
 public:
     SetBridgeOperation(
             const AsteriskSCF::SessionCommunications::V1::AMD_Session_setBridgePtr& cb,
-            const OperationContextPtr& operationContext,
             const SIPSessionPtr& session,
             const AsteriskSCF::SessionCommunications::V1::BridgePrx& bridge,
             const AsteriskSCF::SessionCommunications::V1::SessionListenerPrx& listener,
             const boost::shared_ptr<SIPSessionPriv>& sessionPriv,
             const Ice::Current& current)
         : mCb(cb), 
-          mOperationContext(operationContext), 
           mSession(session), 
           mBridge(bridge), mListener(listener), mImplPriv(sessionPriv), mCurrent(current) { }
     
     SuspendableWorkResult execute(const SuspendableWorkListenerPtr&)
     {
-        AMDContextResultData<SessionInfoPtr, AMD_Session_setBridgePtr>::ptr_type operationCookie = 
-            getContext<AMDContextResultData<SessionInfoPtr, AMD_Session_setBridgePtr> >(
-                mImplPriv->mOperationContextCache,
-                mOperationContext,
-                mCb);
-        if (!operationCookie)
-        {
-            lg(Debug) << "setBridge() detected retry for operation " << mOperationContext->id;
-            return Complete;
-        }
 
         try
         {
             lg(Debug) << "Executing a SetBridge operation";
             mSession->setBridge(mBridge);
             mSession->addListener(mListener);
-            operationCookie->setResult(mSession->getInfo());
+            mCb->ice_response(mSession->getInfo());
         }
         catch (const std::exception& e)
         {
-            operationCookie->setException(ExceptionWrapperPtr(new ExceptionWrapper(e)));
+            mCb->ice_exception(e);
+        }
+        catch (...)
+        {
             assert(false);
+            mCb->ice_exception();
         }
 
         return Complete;
@@ -2349,7 +2381,6 @@ public:
 
 private:
     AsteriskSCF::SessionCommunications::V1::AMD_Session_setBridgePtr mCb;
-    OperationContextPtr mOperationContext;
     SIPSessionPtr mSession;
     AsteriskSCF::SessionCommunications::V1::BridgePrx mBridge;
     AsteriskSCF::SessionCommunications::V1::SessionListenerPrx mListener;
@@ -2367,8 +2398,18 @@ void SIPSession::setBridge_async(
     const AsteriskSCF::SessionCommunications::V1::SessionListenerPrx& listener,
     const Ice::Current& current)
 {
+    AMDContextResultData<SessionInfoPtr, AMD_Session_setBridgePtr>::ptr_type operationCookie = 
+        getContext<AMDContextResultData<SessionInfoPtr, AMD_Session_setBridgePtr> >(
+            mImplPriv->mOperationContextCache,
+            operationContext,
+            cb);
+    if (!operationCookie)
+    {
+        lg(Debug) << "setBridge() detected retry for operation " << operationContext->id;
+        return;
+    }
     lg(Debug) << "queuing a setBridge operation";
-    enqueueSessionWork(new SetBridgeOperation(cb, operationContext, this, bridge, listener, mImplPriv, current));
+    enqueueSessionWork(new SetBridgeOperation(operationCookie->getProxy(), this, bridge, listener, mImplPriv, current));
 }
 
 /**
@@ -2389,45 +2430,36 @@ class RemoveBridgeOperation : public SuspendableWork
 public:
     RemoveBridgeOperation(
             const AsteriskSCF::SessionCommunications::V1::AMD_Session_removeBridgePtr& cb,
-            const OperationContextPtr& operationContext,
             const SIPSessionPtr& session,
             const boost::shared_ptr<SIPSessionPriv>& sessionPriv,
             const AsteriskSCF::SessionCommunications::V1::SessionListenerPrx& listener,
             const Ice::Current& current)
         : mCb(cb), 
-        mOperationContext(operationContext), 
         mSession(session), mImplPriv(sessionPriv), mListener(listener), mCurrent(current) { }
 
     SuspendableWorkResult execute(const SuspendableWorkListenerPtr&)
     {
-        AMDContextData<AMD_Session_removeBridgePtr>::ptr_type operationCookie = 
-            getContext<AMDContextData<AMD_Session_removeBridgePtr> >(
-                mImplPriv->mOperationContextCache,
-                mOperationContext,
-                mCb);
-        if (!operationCookie)
-        {
-            lg(Debug) << "connectStreams() detected retry for operation " << mOperationContext->id;
-            return Complete;
-        }
-
         try
         {
             if (mImplPriv->mBridge == 0)
             {
-                operationCookie->setException(AsteriskSCF::SessionCommunications::V1::NotBridged());
+                mCb->ice_exception(AsteriskSCF::SessionCommunications::V1::NotBridged());
                 return Complete;
             }
 
             mSession->setBridge(0);
 
             mImplPriv->removeListener(mListener);
-            operationCookie->setCompleted();
+            mCb->ice_response();
         }
         catch (const std::exception& e)
         {
-            operationCookie->setException(ExceptionWrapperPtr(new ExceptionWrapper(e)));
+            mCb->ice_exception(e);
+        }
+        catch (...)
+        {
             assert(false);
+            mCb->ice_exception();
         }
 
         return Complete;
@@ -2435,7 +2467,6 @@ public:
 
 private:
     AsteriskSCF::SessionCommunications::V1::AMD_Session_removeBridgePtr mCb;
-    OperationContextPtr mOperationContext;
     SIPSessionPtr mSession;
     boost::shared_ptr<SIPSessionPriv> mImplPriv;
     AsteriskSCF::SessionCommunications::V1::SessionListenerPrx mListener;
@@ -2451,8 +2482,19 @@ void SIPSession::removeBridge_async(
         const AsteriskSCF::SessionCommunications::V1::SessionListenerPrx& listener,
         const Ice::Current& current)
 {
+    AMDContextData<AMD_Session_removeBridgePtr>::ptr_type operationCookie = 
+        getContext<AMDContextData<AMD_Session_removeBridgePtr> >(
+            mImplPriv->mOperationContextCache,
+            operationContext,
+            cb);
+    if (!operationCookie)
+    {
+        lg(Debug) << "connectStreams() detected retry for operation " << operationContext->id;
+        return;
+    }
+
     lg(Debug) << "queuing a removeBridge operation";
-    enqueueSessionWork(new RemoveBridgeOperation(cb, operationContext, this, mImplPriv, listener, current));
+    enqueueSessionWork(new RemoveBridgeOperation(operationCookie->getProxy(), this, mImplPriv, listener, current));
 }
 
 class RemoveListenerOperation : public SuspendableWork
@@ -2516,7 +2558,8 @@ public:
         // be used at all!
         //
         // Record our session within the dialog so code handling pjsip events can do STUFF
-        SIPSessionPtr session = new SIPSession(*mSession);
+        // XXX SIPSessionPtr session = new SIPSession(*mSession);
+        //
 
         // Create the actual INVITE packet
         pjsip_tx_data *packet;
@@ -3044,10 +3087,9 @@ void SIPSession::setCookies(const AsteriskSCF::SessionCommunications::V1::Sessio
 class RemoveCookiesOperation : public SuspendableWork
 {
 public:
-    RemoveCookiesOperation(const OperationContextPtr& operationContext, 
-        const AsteriskSCF::SessionCommunications::V1::SessionCookies& cookieTypes,
+    RemoveCookiesOperation(const AsteriskSCF::SessionCommunications::V1::SessionCookies& cookieTypes,
         const boost::shared_ptr<SIPSessionPriv>& sessionPriv)
-        : mOperationContext(operationContext), mCookieTypes(cookieTypes), mImplPriv(sessionPriv) { }
+        : mCookieTypes(cookieTypes), mImplPriv(sessionPriv) { }
 
     SuspendableWorkResult execute(const SuspendableWorkListenerPtr&)
     {
@@ -3072,7 +3114,6 @@ public:
     }
 
 private:
-    OperationContextPtr mOperationContext;
     AsteriskSCF::SessionCommunications::V1::SessionCookies mCookieTypes;
     boost::shared_ptr<SIPSessionPriv> mImplPriv;
 };
@@ -3091,7 +3132,7 @@ void SIPSession::removeCookies(const OperationContextPtr& operationContext, cons
     }
 
     lg(Debug) << "queuing a removeCookies operation";
-    enqueueSessionWork(new RemoveCookiesOperation(operationContext, cookieTypes, mImplPriv));
+    enqueueSessionWork(new RemoveCookiesOperation(cookieTypes, mImplPriv));
 
     // For this operation, enqueuing the operation is complete enough.
     operationCookie->setCompleted();
diff --git a/src/SIPSession.h b/src/SIPSession.h
index cb131c2..00ea2bd 100644
--- a/src/SIPSession.h
+++ b/src/SIPSession.h
@@ -48,6 +48,14 @@ namespace SIPSessionGateway
 //
 // Forward declarations.
 //
+
+// 
+// XXX: usually this is not sufficient for Ice::Shared derived classes. I
+// suspect that really what happens is that anything that includes SIPSession
+// must also include SIPEndpoint.h if that is truly the case, then SIPSession.h
+// should include SIPEndpoint.h. Bet that is a circular include! Code needs
+// to be reorganized.
+//
 class SIPEndpoint;
 typedef IceUtil::Handle<SIPEndpoint> SIPEndpointPtr;
 
@@ -69,8 +77,11 @@ class SIPSessionPriv;
  * This is where session related work items get enqueued.
  *
  * Each session will have its own SessionWork object to which it will queue suspendable work.
+ * TODO: This class is declared here, but defined in PJSIPSessionModule.cpp. Pretty bad,
+ * code organization wise.
  */
-class SessionWork : virtual public AsteriskSCF::System::WorkQueue::V1::Work, virtual public AsteriskSCF::System::WorkQueue::V1::QueueListener
+class SessionWork : virtual public AsteriskSCF::System::WorkQueue::V1::Work, 
+    virtual public AsteriskSCF::System::WorkQueue::V1::QueueListener
 {
 public:
     /**
@@ -257,6 +268,14 @@ public:
     void getConnectedLine_async(const AsteriskSCF::SessionCommunications::V1::AMD_Session_getConnectedLinePtr& cb, const ::Ice::Current&);
     void getRedirections_async(const AsteriskSCF::SessionCommunications::V1::AMD_Session_getRedirectionsPtr& cb, const Ice::Current& );
 
+    //
+    // TODO: For methods that are meant to be internal only, it is more
+    // "proper" to create a class derived from the class declared in the header
+    // file and includes those methods in *that* class instead. Might seem to
+    // be a bit more work, but if the methods are truly meant to be internal
+    // only, then that's how it is done.
+    //
+
     /**
      * Gets the ConnectedLine for the session.
      * This is an internal method and should only be called from queued operations

-----------------------------------------------------------------------


-- 
asterisk-scf/integration/sip.git



More information about the asterisk-scf-commits mailing list