----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39561/#review103760 -----------------------------------------------------------
trunk/qpid/cpp/src/qpid/broker/amqp/Session.cpp (line 663) <https://reviews.apache.org/r/39561/#comment161840> Sorry for being dense - my caffine level isn't quite where is should be - but if this test passes, isn't the delivery on both the pending and the completed containers? I still have remaining concerns regarding the lifecycle management of pn_link objects. If these seem legit, perhaps they should be tracked in another jira: There are several places where pn_link_closed is called by the Connection processing code due to exception handling, eg: https://github.com/apache/qpid/blob/trunk/qpid/cpp/src/qpid/broker/amqp/Connection.cpp#L594 https://github.com/apache/qpid/blob/trunk/qpid/cpp/src/qpid/broker/amqp/Connection.cpp#L665 https://github.com/apache/qpid/blob/trunk/qpid/cpp/src/qpid/broker/amqp/Connection.cpp#L594 Yet the only call to pn_link_free is here: https://github.com/apache/qpid/blob/trunk/qpid/cpp/src/qpid/broker/amqp/Connection.cpp#L634 The attach/readable/writable Session methods never catch the exception, so I wonder if there is a pn_link_t leak here, or at least the possibility that the related Session/link state is ambiguous and will bite us later. Should the Session trap link-related exceptions and handle cleanup instead? My original patch moved the 'ownership' of the pn_link resource into the Incoming/Outgoing objects, which were tasked with calling pn_link_free (or probably pn_decref might be better?) on destruction. That RAII pattern seems more foolproof with regard to freeing link resources. 2) I'm not convinced this detach processing is legit: https://github.com/apache/qpid/blob/trunk/qpid/cpp/src/qpid/broker/amqp/Connection.cpp#L624 The problem is that qpidd immediately frees the pn_link at the end of the method. From reading the AMQP spec, it seems that qpidd should only free the pn_link after it has sent a detach w/close=true. Sending a detach with close=false indicates the link is still present (though detached) - freeing the link when it is in that state contradicts my interpretation of the spec. - Kenneth Giusti On Oct. 23, 2015, 9:46 a.m., Gordon Sim wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39561/ > ----------------------------------------------------------- > > (Updated Oct. 23, 2015, 9:46 a.m.) > > > Review request for qpid, Alan Conway and Kenneth Giusti. > > > Bugs: QPID-6790 > https://issues.apache.org/jira/browse/QPID-6790 > > > Repository: qpid > > > Description > ------- > > This is an alterntaive fix that simply shortcircuits the asynchornous > completion for deliveries whose link has already been freed. It does this by > tracking deliveries for which asynchronous acceptance is pending, and on > deleting links removes the records of any deliveries affected. On handling an > asynchronous accept, the epnding set is consulted first and the accept only > proceeds if it is found. > > > Diffs > ----- > > trunk/qpid/cpp/src/qpid/broker/amqp/Incoming.cpp 1710066 > trunk/qpid/cpp/src/qpid/broker/amqp/Session.h 1710066 > trunk/qpid/cpp/src/qpid/broker/amqp/Session.cpp 1710066 > > Diff: https://reviews.apache.org/r/39561/diff/ > > > Testing > ------- > > Passes make test. > > > Thanks, > > Gordon Sim > >
