You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@qpid.apache.org by ac...@apache.org on 2014/12/01 18:41:09 UTC
svn commit: r1642720 - in /qpid/trunk/qpid/cpp/src/qpid/messaging/amqp:
AddressHelper.h ConnectionContext.cpp ReceiverContext.cpp ReceiverContext.h
SessionContext.cpp SessionContext.h
Author: aconway
Date: Mon Dec 1 17:41:09 2014
New Revision: 1642720
URL: http://svn.apache.org/r1642720
Log:
QPID-6252: AMQP 1.0 browsing client generates large number of errors on broker.
The problem was that messages for browsing receivers were being recorded on the
client SessionContext unacked list. This is incorrect since you don't ack
browsed messages. They remained on the list after the browsing receiver was
closed, and every subsequent call to acknowledge() on the client would attempt
to ack these messages for a no-longer-existing link. Fix is to not record
browsed messages.
Modified:
qpid/trunk/qpid/cpp/src/qpid/messaging/amqp/AddressHelper.h
qpid/trunk/qpid/cpp/src/qpid/messaging/amqp/ConnectionContext.cpp
qpid/trunk/qpid/cpp/src/qpid/messaging/amqp/ReceiverContext.cpp
qpid/trunk/qpid/cpp/src/qpid/messaging/amqp/ReceiverContext.h
qpid/trunk/qpid/cpp/src/qpid/messaging/amqp/SessionContext.cpp
qpid/trunk/qpid/cpp/src/qpid/messaging/amqp/SessionContext.h
Modified: qpid/trunk/qpid/cpp/src/qpid/messaging/amqp/AddressHelper.h
URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/cpp/src/qpid/messaging/amqp/AddressHelper.h?rev=1642720&r1=1642719&r2=1642720&view=diff
==============================================================================
--- qpid/trunk/qpid/cpp/src/qpid/messaging/amqp/AddressHelper.h (original)
+++ qpid/trunk/qpid/cpp/src/qpid/messaging/amqp/AddressHelper.h Mon Dec 1 17:41:09 2014
@@ -44,6 +44,7 @@ class AddressHelper
const qpid::types::Variant::Map& getNodeProperties() const;
bool getLinkSource(std::string& out) const;
bool getLinkTarget(std::string& out) const;
+ bool getBrowse() const { return browse; }
const qpid::types::Variant::Map& getLinkProperties() const;
static std::string getLinkName(const Address& address);
private:
Modified: qpid/trunk/qpid/cpp/src/qpid/messaging/amqp/ConnectionContext.cpp
URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/cpp/src/qpid/messaging/amqp/ConnectionContext.cpp?rev=1642720&r1=1642719&r2=1642720&view=diff
==============================================================================
--- qpid/trunk/qpid/cpp/src/qpid/messaging/amqp/ConnectionContext.cpp (original)
+++ qpid/trunk/qpid/cpp/src/qpid/messaging/amqp/ConnectionContext.cpp Mon Dec 1 17:41:09 2014
@@ -292,7 +292,7 @@ bool ConnectionContext::get(boost::share
QPID_LOG(debug, "Received message of " << encoded->getSize() << " bytes: ");
encoded->init(impl);
impl.setEncoded(encoded);
- impl.setInternalId(ssn->record(current));
+ impl.setInternalId(ssn->record(current, lnk->getBrowse()));
pn_link_advance(lnk->receiver);
if (lnk->capacity) {
pn_link_flow(lnk->receiver, 1);
Modified: qpid/trunk/qpid/cpp/src/qpid/messaging/amqp/ReceiverContext.cpp
URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/cpp/src/qpid/messaging/amqp/ReceiverContext.cpp?rev=1642720&r1=1642719&r2=1642720&view=diff
==============================================================================
--- qpid/trunk/qpid/cpp/src/qpid/messaging/amqp/ReceiverContext.cpp (original)
+++ qpid/trunk/qpid/cpp/src/qpid/messaging/amqp/ReceiverContext.cpp Mon Dec 1 17:41:09 2014
@@ -36,7 +36,9 @@ ReceiverContext::ReceiverContext(pn_sess
address(a),
helper(address),
receiver(pn_receiver(session, name.c_str())),
- capacity(0), used(0) {}
+ capacity(0), used(0)
+{}
+
ReceiverContext::~ReceiverContext()
{
//pn_link_free(receiver);
Modified: qpid/trunk/qpid/cpp/src/qpid/messaging/amqp/ReceiverContext.h
URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/cpp/src/qpid/messaging/amqp/ReceiverContext.h?rev=1642720&r1=1642719&r2=1642720&view=diff
==============================================================================
--- qpid/trunk/qpid/cpp/src/qpid/messaging/amqp/ReceiverContext.h (original)
+++ qpid/trunk/qpid/cpp/src/qpid/messaging/amqp/ReceiverContext.h Mon Dec 1 17:41:09 2014
@@ -60,6 +60,8 @@ class ReceiverContext
void verify();
Address getAddress() const;
bool hasCurrent();
+ bool getBrowse() const { return helper.getBrowse(); }
+
private:
friend class ConnectionContext;
const std::string name;
Modified: qpid/trunk/qpid/cpp/src/qpid/messaging/amqp/SessionContext.cpp
URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/cpp/src/qpid/messaging/amqp/SessionContext.cpp?rev=1642720&r1=1642719&r2=1642720&view=diff
==============================================================================
--- qpid/trunk/qpid/cpp/src/qpid/messaging/amqp/SessionContext.cpp (original)
+++ qpid/trunk/qpid/cpp/src/qpid/messaging/amqp/SessionContext.cpp Mon Dec 1 17:41:09 2014
@@ -110,10 +110,10 @@ uint32_t SessionContext::getUnsettledAck
return 0;//TODO
}
-qpid::framing::SequenceNumber SessionContext::record(pn_delivery_t* delivery)
+qpid::framing::SequenceNumber SessionContext::record(pn_delivery_t* delivery, bool browse)
{
qpid::framing::SequenceNumber id = next++;
- unacked[id] = delivery;
+ if (!browse) unacked[id] = delivery;
QPID_LOG(debug, "Recorded delivery " << id << " -> " << delivery);
return id;
}
Modified: qpid/trunk/qpid/cpp/src/qpid/messaging/amqp/SessionContext.h
URL: http://svn.apache.org/viewvc/qpid/trunk/qpid/cpp/src/qpid/messaging/amqp/SessionContext.h?rev=1642720&r1=1642719&r2=1642720&view=diff
==============================================================================
--- qpid/trunk/qpid/cpp/src/qpid/messaging/amqp/SessionContext.h (original)
+++ qpid/trunk/qpid/cpp/src/qpid/messaging/amqp/SessionContext.h Mon Dec 1 17:41:09 2014
@@ -75,7 +75,7 @@ class SessionContext
qpid::framing::SequenceNumber next;
std::string name;
- qpid::framing::SequenceNumber record(pn_delivery_t*);
+ qpid::framing::SequenceNumber record(pn_delivery_t*, bool browse);
void acknowledge();
void acknowledge(const qpid::framing::SequenceNumber& id, bool cummulative);
void acknowledge(DeliveryMap::iterator begin, DeliveryMap::iterator end);
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@qpid.apache.org
For additional commands, e-mail: commits-help@qpid.apache.org
Re: svn commit: r1642720 - in
/qpid/trunk/qpid/cpp/src/qpid/messaging/amqp: AddressHelper.h
ConnectionContext.cpp ReceiverContext.cpp ReceiverContext.h
SessionContext.cpp SessionContext.h
Posted by Alan Conway <ac...@redhat.com>.
I have a new and satisfyingly simpler fix for this that I think covers all the points raised on the mailing list. Will commit as soon as apache SVN is back in action.
----- Original Message -----
> On 12/02/2014 07:57 PM, Rob Godfrey wrote:
> > From a protocol perspective all transfers need to be settled. Does the
> > broker support the "send settled" mode?
>
> Yes
>
> > The sane thing for browsing
> > consumers would be to suggest to the broker that it should use
> > "snd-settle-mode" of settled.
>
> Yes, that would I think be relatively straightforward.Its what is done
> if the link is configured to be 'unreliable'.
>
> > The client would only need to
> > (automatically, behind the scenes) settle transfers if the broker opted not
> > to use that settlement mode.
>
> I think proton may require it to be locally settled anyway, just so it
> can be freed.
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
> For additional commands, e-mail: dev-help@qpid.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
Re: svn commit: r1642720 - in /qpid/trunk/qpid/cpp/src/qpid/messaging/amqp:
AddressHelper.h ConnectionContext.cpp ReceiverContext.cpp ReceiverContext.h
SessionContext.cpp SessionContext.h
Posted by Gordon Sim <gs...@redhat.com>.
On 12/02/2014 07:57 PM, Rob Godfrey wrote:
> From a protocol perspective all transfers need to be settled. Does the
> broker support the "send settled" mode?
Yes
> The sane thing for browsing
> consumers would be to suggest to the broker that it should use
> "snd-settle-mode" of settled.
Yes, that would I think be relatively straightforward.Its what is done
if the link is configured to be 'unreliable'.
> The client would only need to
> (automatically, behind the scenes) settle transfers if the broker opted not
> to use that settlement mode.
I think proton may require it to be locally settled anyway, just so it
can be freed.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
Re: svn commit: r1642720 - in /qpid/trunk/qpid/cpp/src/qpid/messaging/amqp:
AddressHelper.h ConnectionContext.cpp ReceiverContext.cpp ReceiverContext.h
SessionContext.cpp SessionContext.h
Posted by Rob Godfrey <ro...@gmail.com>.
>From a protocol perspective all transfers need to be settled. Does the
broker support the "send settled" mode? The sane thing for browsing
consumers would be to suggest to the broker that it should use
"snd-settle-mode" of settled. The client would only need to
(automatically, behind the scenes) settle transfers if the broker opted not
to use that settlement mode.
-- Rob
On 2 December 2014 at 20:49, Alan Conway <ac...@redhat.com> wrote:
> On Tue, 2014-12-02 at 17:57 +0000, Gordon Sim wrote:
> > On 12/02/2014 05:38 PM, Gordon Sim wrote:
> > > On 12/01/2014 05:41 PM, aconway@apache.org wrote:
> > >> Author: aconway
> > >> Date: Mon Dec 1 17:41:09 2014
> > >> New Revision: 1642720
> > >>
> > >> URL: http://svn.apache.org/r1642720
> > >> Log:
> > >> QPID-6252: AMQP 1.0 browsing client generates large number of errors
> > >> on broker.
> > >>
> > >> The problem was that messages for browsing receivers were being
> > >> recorded on the
> > >> client SessionContext unacked list. This is incorrect since you don't
> ack
> > >> browsed messages. They remained on the list after the browsing
> > >> receiver was
> > >> closed, and every subsequent call to acknowledge() on the client would
> > >> attempt
> > >> to ack these messages for a no-longer-existing link. Fix is to not
> record
> > >> browsed messages.
> > >
> > > I don't think this is right.
> > >
> > > From the client's perspective, it should still be able to accept and
> > > settle deliveries, regardless of whether the broker will dequeue in
> > > response.
> > >
> > > From the protocol perspective, deliveries must always get settled.
> > > Otherwise they need to be tracked (whether within proton or on the
> > > sending peer).
> > >
> > > As regards the original error, the spec says:
> > >
> > > The disposition performative MAY refer to deliveries on
> > > links that are no longer attached. As long as the links
> > > have not been closed or detached with an error then the
> > > deliveries are still “live” and the updated state MUST
> > > be applied.
> > >
> > > It doesn't explicitly say what should happen if the link has been
> closed
> > > (not just detached), so its probably best to just ignore it on the
> > > broker (change the log level from error to info perhaps).
> > >
> > > The client should probably also settle all existing deliveries received
> > > on a link before closing it.
> >
> > Or perhaps at least remove the deliveries from the unacked list if their
> > associated link is closed.
> >
>
> The client definitely needs to do something with these deliveries,
> otherwise they accumulate on the session and you get longer and longer
> lists of bad acknowledgements as time goes on. I don't think anyone on
> the user interface side expects that they can/should ack messages from a
> browsing session, although I would think doing so should be a no-op not
> an error. Are you saying that the protocol requires it? From my reading
> the stuff about settlement only applies to "acquired transfers", which I
> took to exclude transfers from a link that is in COPY mode.
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
> For additional commands, e-mail: dev-help@qpid.apache.org
>
>
Re: svn commit: r1642720 - in /qpid/trunk/qpid/cpp/src/qpid/messaging/amqp:
AddressHelper.h ConnectionContext.cpp ReceiverContext.cpp ReceiverContext.h
SessionContext.cpp SessionContext.h
Posted by Gordon Sim <gs...@redhat.com>.
On 12/02/2014 07:49 PM, Alan Conway wrote:
> On Tue, 2014-12-02 at 17:57 +0000, Gordon Sim wrote:
>> On 12/02/2014 05:38 PM, Gordon Sim wrote:
>>> On 12/01/2014 05:41 PM, aconway@apache.org wrote:
>>>> Author: aconway
>>>> Date: Mon Dec 1 17:41:09 2014
>>>> New Revision: 1642720
>>>>
>>>> URL: http://svn.apache.org/r1642720
>>>> Log:
>>>> QPID-6252: AMQP 1.0 browsing client generates large number of errors
>>>> on broker.
>>>>
>>>> The problem was that messages for browsing receivers were being
>>>> recorded on the
>>>> client SessionContext unacked list. This is incorrect since you don't ack
>>>> browsed messages. They remained on the list after the browsing
>>>> receiver was
>>>> closed, and every subsequent call to acknowledge() on the client would
>>>> attempt
>>>> to ack these messages for a no-longer-existing link. Fix is to not record
>>>> browsed messages.
>>>
>>> I don't think this is right.
>>>
>>> From the client's perspective, it should still be able to accept and
>>> settle deliveries, regardless of whether the broker will dequeue in
>>> response.
>>>
>>> From the protocol perspective, deliveries must always get settled.
>>> Otherwise they need to be tracked (whether within proton or on the
>>> sending peer).
>>>
>>> As regards the original error, the spec says:
>>>
>>> The disposition performative MAY refer to deliveries on
>>> links that are no longer attached. As long as the links
>>> have not been closed or detached with an error then the
>>> deliveries are still “live” and the updated state MUST
>>> be applied.
>>>
>>> It doesn't explicitly say what should happen if the link has been closed
>>> (not just detached), so its probably best to just ignore it on the
>>> broker (change the log level from error to info perhaps).
>>>
>>> The client should probably also settle all existing deliveries received
>>> on a link before closing it.
>>
>> Or perhaps at least remove the deliveries from the unacked list if their
>> associated link is closed.
>>
>
> The client definitely needs to do something with these deliveries,
> otherwise they accumulate on the session and you get longer and longer
> lists of bad acknowledgements as time goes on. I don't think anyone on
> the user interface side expects that they can/should ack messages from a
> browsing session, although I would think doing so should be a no-op not
> an error. Are you saying that the protocol requires it?
Yes, proton does also. However we could just settle at the point the
delivery is received and not add to the unacked list at all (as you do
now). As the code stands now that would be sufficient. Ideally I think
acknowledge is compatible with browse, it just serves a different
purpose. Rather than directly causing a message to be dequeued, it
signals to the broker that this subscriber has seen it. A future
implementation on the/a broker could then use that to determine when all
registered subscribers have seen it.
> From my reading
> the stuff about settlement only applies to "acquired transfers", which I
> took to exclude transfers from a link that is in COPY mode.
You could argue that the accept outcome is only relevant to acquired
transfers (in my view it's not that clear in the spec text). However
settlement is distinct from that.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
Re: svn commit: r1642720 - in
/qpid/trunk/qpid/cpp/src/qpid/messaging/amqp: AddressHelper.h
ConnectionContext.cpp ReceiverContext.cpp ReceiverContext.h
SessionContext.cpp SessionContext.h
Posted by Alan Conway <ac...@redhat.com>.
On Tue, 2014-12-02 at 17:57 +0000, Gordon Sim wrote:
> On 12/02/2014 05:38 PM, Gordon Sim wrote:
> > On 12/01/2014 05:41 PM, aconway@apache.org wrote:
> >> Author: aconway
> >> Date: Mon Dec 1 17:41:09 2014
> >> New Revision: 1642720
> >>
> >> URL: http://svn.apache.org/r1642720
> >> Log:
> >> QPID-6252: AMQP 1.0 browsing client generates large number of errors
> >> on broker.
> >>
> >> The problem was that messages for browsing receivers were being
> >> recorded on the
> >> client SessionContext unacked list. This is incorrect since you don't ack
> >> browsed messages. They remained on the list after the browsing
> >> receiver was
> >> closed, and every subsequent call to acknowledge() on the client would
> >> attempt
> >> to ack these messages for a no-longer-existing link. Fix is to not record
> >> browsed messages.
> >
> > I don't think this is right.
> >
> > From the client's perspective, it should still be able to accept and
> > settle deliveries, regardless of whether the broker will dequeue in
> > response.
> >
> > From the protocol perspective, deliveries must always get settled.
> > Otherwise they need to be tracked (whether within proton or on the
> > sending peer).
> >
> > As regards the original error, the spec says:
> >
> > The disposition performative MAY refer to deliveries on
> > links that are no longer attached. As long as the links
> > have not been closed or detached with an error then the
> > deliveries are still “live” and the updated state MUST
> > be applied.
> >
> > It doesn't explicitly say what should happen if the link has been closed
> > (not just detached), so its probably best to just ignore it on the
> > broker (change the log level from error to info perhaps).
> >
> > The client should probably also settle all existing deliveries received
> > on a link before closing it.
>
> Or perhaps at least remove the deliveries from the unacked list if their
> associated link is closed.
>
The client definitely needs to do something with these deliveries,
otherwise they accumulate on the session and you get longer and longer
lists of bad acknowledgements as time goes on. I don't think anyone on
the user interface side expects that they can/should ack messages from a
browsing session, although I would think doing so should be a no-op not
an error. Are you saying that the protocol requires it? From my reading
the stuff about settlement only applies to "acquired transfers", which I
took to exclude transfers from a link that is in COPY mode.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
Re: svn commit: r1642720 - in /qpid/trunk/qpid/cpp/src/qpid/messaging/amqp:
AddressHelper.h ConnectionContext.cpp ReceiverContext.cpp ReceiverContext.h
SessionContext.cpp SessionContext.h
Posted by Gordon Sim <gs...@redhat.com>.
On 12/02/2014 05:38 PM, Gordon Sim wrote:
> On 12/01/2014 05:41 PM, aconway@apache.org wrote:
>> Author: aconway
>> Date: Mon Dec 1 17:41:09 2014
>> New Revision: 1642720
>>
>> URL: http://svn.apache.org/r1642720
>> Log:
>> QPID-6252: AMQP 1.0 browsing client generates large number of errors
>> on broker.
>>
>> The problem was that messages for browsing receivers were being
>> recorded on the
>> client SessionContext unacked list. This is incorrect since you don't ack
>> browsed messages. They remained on the list after the browsing
>> receiver was
>> closed, and every subsequent call to acknowledge() on the client would
>> attempt
>> to ack these messages for a no-longer-existing link. Fix is to not record
>> browsed messages.
>
> I don't think this is right.
>
> From the client's perspective, it should still be able to accept and
> settle deliveries, regardless of whether the broker will dequeue in
> response.
>
> From the protocol perspective, deliveries must always get settled.
> Otherwise they need to be tracked (whether within proton or on the
> sending peer).
>
> As regards the original error, the spec says:
>
> The disposition performative MAY refer to deliveries on
> links that are no longer attached. As long as the links
> have not been closed or detached with an error then the
> deliveries are still “live” and the updated state MUST
> be applied.
>
> It doesn't explicitly say what should happen if the link has been closed
> (not just detached), so its probably best to just ignore it on the
> broker (change the log level from error to info perhaps).
>
> The client should probably also settle all existing deliveries received
> on a link before closing it.
Or perhaps at least remove the deliveries from the unacked list if their
associated link is closed.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
Re: svn commit: r1642720 - in /qpid/trunk/qpid/cpp/src/qpid/messaging/amqp:
AddressHelper.h ConnectionContext.cpp ReceiverContext.cpp ReceiverContext.h
SessionContext.cpp SessionContext.h
Posted by Gordon Sim <gs...@redhat.com>.
On 12/01/2014 05:41 PM, aconway@apache.org wrote:
> Author: aconway
> Date: Mon Dec 1 17:41:09 2014
> New Revision: 1642720
>
> URL: http://svn.apache.org/r1642720
> Log:
> QPID-6252: AMQP 1.0 browsing client generates large number of errors on broker.
>
> The problem was that messages for browsing receivers were being recorded on the
> client SessionContext unacked list. This is incorrect since you don't ack
> browsed messages. They remained on the list after the browsing receiver was
> closed, and every subsequent call to acknowledge() on the client would attempt
> to ack these messages for a no-longer-existing link. Fix is to not record
> browsed messages.
I don't think this is right.
From the client's perspective, it should still be able to accept and
settle deliveries, regardless of whether the broker will dequeue in
response.
From the protocol perspective, deliveries must always get settled.
Otherwise they need to be tracked (whether within proton or on the
sending peer).
As regards the original error, the spec says:
The disposition performative MAY refer to deliveries on
links that are no longer attached. As long as the links
have not been closed or detached with an error then the
deliveries are still “live” and the updated state MUST
be applied.
It doesn't explicitly say what should happen if the link has been closed
(not just detached), so its probably best to just ignore it on the
broker (change the log level from error to info perhaps).
The client should probably also settle all existing deliveries received
on a link before closing it.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org