----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39427/#review103548 -----------------------------------------------------------
trunk/qpid/cpp/src/qpid/broker/amqp/Session.cpp (line 615) <https://reviews.apache.org/r/39427/#comment161601> Not clear what you intend here. Is this all to be removed? trunk/qpid/cpp/src/qpid/broker/amqp/Session.cpp (line 705) <https://reviews.apache.org/r/39427/#comment161602> Is there ever a case where an exception is raised (thus closing the link) and yet isLinkDone() would return false? The change implies that there is, buT I can't see what that would be and it makes me a little nervous. trunk/qpid/cpp/src/qpid/broker/amqp/Session.cpp (line 708) <https://reviews.apache.org/r/39427/#comment161599> Minor issue, but I prefer an if/else rather than the plain if and a continue. To me it seems clearer on the two paths for iterating forward. trunk/qpid/cpp/src/qpid/broker/amqp/Session.cpp (line 1006) <https://reviews.apache.org/r/39427/#comment161603> This method doesn't distinguish between 'detach' and 'close' (where the latter is a detach with closed=true). Is this used only in some cases? Or is it ended to be the sole path through which links are closed? I feel nervous about this checking of queued, credit, unsettled etc. If there is stuff we need to do before closing the link, e.g. explicitly settling deliveries and nulling out any reference to them, I'd prefer we ensure that is done rather than querying proton. - Gordon Sim On Oct. 21, 2015, 5:51 p.m., Kenneth Giusti wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39427/ > ----------------------------------------------------------- > > (Updated Oct. 21, 2015, 5:51 p.m.) > > > Review request for qpid and Gordon Sim. > > > Repository: qpid > > > Description > ------- > > WIP > > This patch attempts to keep the proton link active until there is no more > pending work that can be completed. > > It consists of two changes: > > 1) Move ownership of the pn_link_t resource into its respective > Incoming/Outgoing instance. The Incoming/Outgoing instance pn_link_free's > the link on destruction > > 2) establish a detach protocol between the links and the owning Session. > When the peer sends a DETACH, the corresponding link is notified of the > detach. Once that happens, it must complete any outstanding work (eg. send > pending msgs w/remaining credit, settle incoming deliveries). Once all the > work is finished, the Session can drop the shared_ptr to the Out/In link > object and it's destructor will be called. > > This is based on my understanding of the existing code - which may be > entirely wrong. > > > Diffs > ----- > > trunk/qpid/cpp/src/qpid/broker/amqp/Connection.cpp 1708221 > trunk/qpid/cpp/src/qpid/broker/amqp/Incoming.h 1708221 > trunk/qpid/cpp/src/qpid/broker/amqp/Incoming.cpp 1708221 > trunk/qpid/cpp/src/qpid/broker/amqp/Outgoing.h 1708221 > trunk/qpid/cpp/src/qpid/broker/amqp/Outgoing.cpp 1708221 > trunk/qpid/cpp/src/qpid/broker/amqp/Relay.h 1708221 > trunk/qpid/cpp/src/qpid/broker/amqp/Relay.cpp 1708221 > trunk/qpid/cpp/src/qpid/broker/amqp/Session.h 1708221 > trunk/qpid/cpp/src/qpid/broker/amqp/Session.cpp 1708221 > > Diff: https://reviews.apache.org/r/39427/diff/ > > > Testing > ------- > > The interopt tests pass on the system they previously failed on. I'm still > seeing failures in the ha_tests which may be related to this patch. Just > sending it out for early feedback. > > > Thanks, > > Kenneth Giusti > >
