> On Oct. 23, 2015, 12:55 p.m., Kenneth Giusti wrote: > > trunk/qpid/cpp/src/qpid/broker/amqp/Session.cpp, line 663 > > <https://reviews.apache.org/r/39561/diff/3/?file=1103979#file1103979line663> > > > > 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?
Yes. If then as Alan had hypothesised, the link gets deleted before we process completed, the delivery will have been removed from pending, so when we come to act on the record in completed we see its no longer in pending and don't proceed. On Oct. 23, 2015, 12:55 p.m., Gordon Sim wrote: > > 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. If the broker initiates the detach, then the client should respond by also detaching, which should then result in doLinkRemoteDetach() being called which would then free the link. On the second point, the pn_link_t object is not the same as the logical link, so freeing it locally doesn't imply anything other than that the broker is finished with that object and won't use it again. If the client then reattached using the same link name and container id (perhaps on a different connection) a new pn_link_t would be created to represent that link, even if then associated with some other broker resident terminus state. - Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39561/#review103760 ----------------------------------------------------------- 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 > >
