You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Kenneth Giusti <kg...@apache.org> on 2015/10/19 03:31:58 UTC
Review Request 39427: Do not free proton links until there is no more
pending work on the link.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39427/
-----------------------------------------------------------
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.
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
Re: Review Request 39427: Do not free proton links until there is no
more pending work on the link.
Posted by Alan Conway <ac...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39427/#review103445
-----------------------------------------------------------
It looks plausible but I don't know the code well enough to be confident.
- Alan Conway
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
>
>
Re: Review Request 39427: Do not free proton links until there is no
more pending work on the link.
Posted by Kenneth Giusti <kg...@apache.org>.
> On Oct. 22, 2015, 9:38 a.m., Gordon Sim wrote:
> > trunk/qpid/cpp/src/qpid/broker/amqp/Session.cpp, line 617
> > <https://reviews.apache.org/r/39427/diff/1/?file=1100663#file1100663line617>
> >
> > Not clear what you intend here. Is this all to be removed?
Yes it needs to be removed. I kept it around so I could reference the original code and meant to take it out.
> On Oct. 22, 2015, 9:38 a.m., Gordon Sim wrote:
> > trunk/qpid/cpp/src/qpid/broker/amqp/Session.cpp, line 709
> > <https://reviews.apache.org/r/39427/diff/1/?file=1100663#file1100663line709>
> >
> > 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.
Yes - I find this change too messy.
I beginning to think that letting the Session object directly access the pn_link_t may not be the right approach. Maybe it is best to encapsulate the pn_link in the Out/In* classes and provide interfaces for the Session. The In/Out* classes (or better yet a common base class) could provide a "link abort" interface for the Session to call on an exception (rather than calling 'detached'). In the case of an exception qpidd should immediately close the link and discard any internal state/data associated with that link, since an orderly clean close is probably not possible.
All the logic for knowing when a link is actually "done" and can be erased would be encapsulated in the link object itself. And the link would guarantee that it's state would reach 'done' if either a detach or abort occurred.
> On Oct. 22, 2015, 9:38 a.m., Gordon Sim wrote:
> > trunk/qpid/cpp/src/qpid/broker/amqp/Session.cpp, line 712
> > <https://reviews.apache.org/r/39427/diff/1/?file=1100663#file1100663line712>
> >
> > 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.
Agreed - will fix.
> On Oct. 22, 2015, 9:38 a.m., Gordon Sim wrote:
> > trunk/qpid/cpp/src/qpid/broker/amqp/Session.cpp, line 1011
> > <https://reviews.apache.org/r/39427/diff/1/?file=1100663#file1100663line1011>
> >
> > 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.
I think moving this processing into the In/Out* classes 'detach' and 'abort' handlers would be a good start, since these objects hold the deliveries that need to be released and will know when the link is actually 'done'.
- Kenneth
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39427/#review103548
-----------------------------------------------------------
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
>
>
Re: Review Request 39427: Do not free proton links until there is no
more pending work on the link.
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 39427: Do not free proton links until there is no
more pending work on the link.
Posted by Kenneth Giusti <kg...@apache.org>.
-----------------------------------------------------------
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 (updated)
-------
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