----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/10704/#review19736 -----------------------------------------------------------
I don't see anything wrong but on fetchImpl I'm not familiar enough with the code to be sure if it is safe. /trunk/qpid/cpp/src/qpid/client/amqp0_10/ReceiverImpl.cpp <https://reviews.apache.org/r/10704/#comment40727> It's not immediately obvious that this is safe - could there be code that depends on the 3 actions (release, cancel, receiverCancelled) be atomic? In other words is the fetchImpl in a valid state at each unlock, and can all other concurrent functions safely be called during the unlocks? - Alan Conway On April 22, 2013, 3:52 p.m., Gordon Sim wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/10704/ > ----------------------------------------------------------- > > (Updated April 22, 2013, 3:52 p.m.) > > > Review request for qpid and Alan Conway. > > > Description > ------- > > The changes involve: > > (1) removing the unnecessary lock from ReceiverImpl::getName() since the > destination member variable is const > > (2) removing the lock from the two forms of SessionImpl:acknowledgeImpl() > since the transactional member is const and IncomingMessages::accept() does > its own locking > > (3) releasing the lock while calling back from ReceiverImpl to SessionImpl in > ReceiverImpl::close() > > These help break the possible deadlock. > > > This addresses bug QPID-4764. > https://issues.apache.org/jira/browse/QPID-4764 > > > Diffs > ----- > > /trunk/qpid/cpp/src/qpid/client/amqp0_10/ReceiverImpl.cpp 1470466 > /trunk/qpid/cpp/src/qpid/client/amqp0_10/SessionImpl.cpp 1470466 > > Diff: https://reviews.apache.org/r/10704/diff/ > > > Testing > ------- > > make check passes (I couldn't reproduce the deadlock). > > > Thanks, > > Gordon Sim > >
