You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Gordon Sim <gs...@redhat.com> on 2009/04/03 19:53:19 UTC
[c++]: race condition in client when handling session close due to
execution.exception
ConnectionImpl holds weak_ptrs to SessionImpl instances. If all the
Session objects referring to a given SessionImpl are allowed to go out
of scope, ConnectionImpl treats the session as having been detached and
throws a NotAttachedException if there are any incoming frames for the
channel the session was attached to.
This is particularly problematic when the session is sent an
execution.exception by the broker. This results in an exception being
thrown to the application which may well as a result let the session go
out of scope.
However, following the execution.exception is the session.detach as
required by the AMQP 0-10 spec. If the session goes out of scope before
this is handled, then the whole connection ends up being destroyed due
to the NotAttachedException thrown.
I've raised a jira: https://issues.apache.org/jira/browse/QPID-1785. I
also have a proposed 'fix' (see patch below). This just prevents the
NotAttachedException being thrown, logging a message instead.
It does mean that the client would not correctly handle an erroneous
attempt by the broker to send frames on a session that has genuinely
been detached. That isn't as serious in my view and seems like a
worthwhile trade for getting the issue describe in the jira resolved.
Consequently unless there are objections I'll go ahead and apply this patch.
Index: src/qpid/client/ConnectionImpl.cpp
===================================================================
--- src/qpid/client/ConnectionImpl.cpp (revision 761362)
+++ src/qpid/client/ConnectionImpl.cpp (working copy)
@@ -111,9 +111,11 @@
Mutex::ScopedLock l(lock);
s = sessions[frame.getChannel()].lock();
}
- if (!s)
- throw NotAttachedException(QPID_MSG("Invalid channel: " <<
frame.getChannel()));
- s->in(frame);
+ if (!s) {
+ QPID_LOG(info, "Dropping frame received on invalid channel: "
<< frame);
+ } else {
+ s->in(frame);
+ }
}
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
Re: [c++]: race condition in client when handling session close due
to execution.exception
Posted by Gordon Sim <gs...@redhat.com>.
James Mansion wrote:
> Gordon Sim wrote:
>> However, following the execution.exception is the session.detach as
>> required by the AMQP 0-10 spec. If the session goes out of scope
>> before this is handled, then the whole connection ends up being
>> destroyed due to the NotAttachedException thrown.
> To me that suggests that some representation of a Channel object *must*
> live on until the client and server have both agreed that it has been
> closed,
Yes, I agree. It is logically incorrect to treat a session that the
application has let go out of scope as detached without ensuring that
the server has been informed of the detachment[1].
> otherwise something might cause the client to try to reuse the
> channel while there are still messages in transit for it.
True. The current API doesn't yet support reusing channels on a given
connection, though[2].
>> It does mean that the client would not correctly handle an erroneous
>> attempt by the broker to send frames on a session that has genuinely
>> been detached. That isn't as serious in my view and seems like a
>> worthwhile trade for getting the issue describe in the jira resolved.
> Surely the problem is also that the broker has (or will shortly) send
> frames that the protocol requires - that's hardly erroneous?
After sending an execution.exception, the broker will only send the
detach frame. The current client implementation takes the detach as
implied by the exception and marks the session as detached on handling
the exception. Therefore the detach frame will not have actually have
any effect. Dropping it is in my view preferable to throwing an
exception, which will result in the whole connection being lost.
Correcting the design is even better of course. My proposed patch
doesn't attempt to do that, it is just a simple fix for an immediate and
real problem. However I've created a couple of Jiras for these other
issues if anyone wants to take them on.
[1] https://issues.apache.org/jira/browse/QPID-1789
[2] https://issues.apache.org/jira/browse/QPID-1788
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
Re: [c++]: race condition in client when handling session close due
to execution.exception
Posted by James Mansion <ja...@mansionfamily.plus.com>.
Gordon Sim wrote:
> However, following the execution.exception is the session.detach as
> required by the AMQP 0-10 spec. If the session goes out of scope
> before this is handled, then the whole connection ends up being
> destroyed due to the NotAttachedException thrown.
To me that suggests that some representation of a Channel object *must*
live on until the client and server have both agreed that it has been
closed, otherwise something might cause the client to try to reuse the
channel while there are still messages in transit for it. I think the
time to do this is when the use-count is going to zero - depending on
how the smart pointer works this will either be just in time and the
channel can be marked as a zombie, or just too late and you'll need to
insert a stand-in zombie.
> It does mean that the client would not correctly handle an erroneous
> attempt by the broker to send frames on a session that has genuinely
> been detached. That isn't as serious in my view and seems like a
> worthwhile trade for getting the issue describe in the jira resolved.
Surely the problem is also that the broker has (or will shortly) send
frames that the protocol requires - that's hardly erroneous?
James
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
Re: [c++]: race condition in client when handling session close due
to execution.exception
Posted by Alan Conway <ac...@redhat.com>.
Gordon Sim wrote:
> ConnectionImpl holds weak_ptrs to SessionImpl instances. If all the
> Session objects referring to a given SessionImpl are allowed to go out
> of scope, ConnectionImpl treats the session as having been detached and
> throws a NotAttachedException if there are any incoming frames for the
> channel the session was attached to.
>
You might want to consider https://bugzilla.redhat.com/show_bug.cgi?id=488942 as
well when looking at this.
I think the clients connection/session model is in need of a re-think.
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org