You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by "Ted Ross (JIRA)" <qp...@incubator.apache.org> on 2010/01/07 21:38:54 UTC
[jira] Created: (QPID-2335) Arguments for a binding to an XML
exchange are not visible through management
Arguments for a binding to an XML exchange are not visible through management
-----------------------------------------------------------------------------
Key: QPID-2335
URL: https://issues.apache.org/jira/browse/QPID-2335
Project: Qpid
Issue Type: Bug
Components: C++ Broker
Affects Versions: 0.6
Reporter: Ted Ross
Priority: Minor
If you create a binding to the XML exchange (using python/examples/xml-exchange/declare_queues.py for example), then look at the binding using qpid-tool, the arguments field is {} (an empty map).
Arguments for headers exchanges can be viewed correctly, this only affects the xml exchange.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
[jira] Commented: (QPID-2335) Arguments for a binding to an XML
exchange are not visible through management
Posted by "john dunning (JIRA)" <qp...@incubator.apache.org>.
[ https://issues.apache.org/jira/browse/QPID-2335?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12799837#action_12799837 ]
john dunning commented on QPID-2335:
------------------------------------
This turns out be rather larger than the bug description says.
The root of the problem is that as currently constituted, the XmlBinding has its own constructor which takes different args than the other Binding classes. In particular, in place of the bindgingArguments arg, it has a query, which is meant to be the pre-parsed XQQuery. That's why the binding object doesn't show its args, they were never passed in.
It's relatively straightforward to change the constructor so as to adhere to the same protocol as the rest of them. That implies that the body of the constructor parses the string into a query, and stores the query in the binding object. The parent class constructor stores bindingArguments, and everybody's happy.
However, that causes a different problem. The parent class's (Binding) constructor, as a side-effect, stores the object in the managed objects database. See src/qpid/broker/Exchange.cpp:328. This means that if there's a derived class whose constructor does anything to the state of the object, you have a race condition, where somebody else gets their hands on this object before the guy running the constructor has finished running it. I've observed this while debugging my patch for the original problem.
It seems to me that it's risky to assume that constructors of derived classes cannot alter the state of an object after the base class constructor has run. I propose to fix this by removing the implicit addObject, and making it explicit. This means that all the callers of constructors of bindings must now addObject themselves, which touches a lot of files. The upside is that it's a little more obvious from looking at the code what's going on, and therefore less prone to error.
There are other variations on that theme; I could rejigger the classes so that only derived classes call addObject from their constructors. That would mean that there's a new class to replace Binding, which is probably just as big a change. That also still leaves us open to a repeat of this bug if somebody down the road derives from one of these new classes. I could also find a way to interlock things; there are plenty of locks around, and I couldn introduce a new one, essentially locking the global datastore while anybody is running a constructor. Given the frequency with which things are getting created, that's probably not desirable either.
There's another way of fixing the top-level problem, namely go back to pre-parsing the query, and only call the constructor when it's ready to go. IOW, go back to the model where the constructor doesn't do any work. This is less intrusive, but leaves the core assumption in place, with the associated likelihood that somebody down the road will make a constructor which does work, and trip over the same bug.
Comments welcome, particularly if anybody thinks there's a less intrusive way to make this solid.
> Arguments for a binding to an XML exchange are not visible through management
> -----------------------------------------------------------------------------
>
> Key: QPID-2335
> URL: https://issues.apache.org/jira/browse/QPID-2335
> Project: Qpid
> Issue Type: Bug
> Components: C++ Broker
> Affects Versions: 0.6
> Reporter: Ted Ross
> Assignee: Ted Ross
> Priority: Minor
>
> If you create a binding to the XML exchange (using python/examples/xml-exchange/declare_queues.py for example), then look at the binding using qpid-tool, the arguments field is {} (an empty map).
> Arguments for headers exchanges can be viewed correctly, this only affects the xml exchange.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
[jira] Commented: (QPID-2335) Arguments for a binding to an XML
exchange are not visible through management
Posted by "john dunning (JIRA)" <qp...@incubator.apache.org>.
[ https://issues.apache.org/jira/browse/QPID-2335?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12800335#action_12800335 ]
john dunning commented on QPID-2335:
------------------------------------
Yes, I thought about (and tried) making the cosntructor for XmlBinding mimic the others, but take an extra arg for query. That works, in the sense that it fixes the original bug. That wasn't my first choice, because it seemed wrong to (somewhat gratuitously) make that class have a constructor with a different signature that his siblings, but maybe we don't care. If the community feels like it's more tasteful to do it that way than to do the parsing in the constructor, that's fine with me.
You're also correct that the issue about when the registration of the object happens w/r/t the constructor deserves a separate bug. I'll clone this one or open a new one or something along in here for that issue. But see below...
Re the bigger concern, it wasn't simply about access to the mgmtobject, but about access to the binding itself. If you've stashed that object in some global location where other threads can get at it, prior to finishing execution of all the parts of the constructor, you've got a race condition. That's the situation I thought I had with my modified constructor. I was sure I'd tripped over that in the process of running some test code.
Since then, I changed some stuff around, and svn updated, and now I can't reproduce that error any more. I need to do some more careful testing and code inspection, and try to determine whether the bug is really there (meaning I was unlucky before, and got lucky now) or whether I was confusing myself with something that just looked like the race condition, but wasn't really.
> Arguments for a binding to an XML exchange are not visible through management
> -----------------------------------------------------------------------------
>
> Key: QPID-2335
> URL: https://issues.apache.org/jira/browse/QPID-2335
> Project: Qpid
> Issue Type: Bug
> Components: C++ Broker
> Affects Versions: 0.6
> Reporter: Ted Ross
> Assignee: Ted Ross
> Priority: Minor
>
> If you create a binding to the XML exchange (using python/examples/xml-exchange/declare_queues.py for example), then look at the binding using qpid-tool, the arguments field is {} (an empty map).
> Arguments for headers exchanges can be viewed correctly, this only affects the xml exchange.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
[jira] Assigned: (QPID-2335) Arguments for a binding to an XML
exchange are not visible through management
Posted by "Ted Ross (JIRA)" <qp...@incubator.apache.org>.
[ https://issues.apache.org/jira/browse/QPID-2335?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ted Ross reassigned QPID-2335:
------------------------------
Assignee: Ted Ross
> Arguments for a binding to an XML exchange are not visible through management
> -----------------------------------------------------------------------------
>
> Key: QPID-2335
> URL: https://issues.apache.org/jira/browse/QPID-2335
> Project: Qpid
> Issue Type: Bug
> Components: C++ Broker
> Affects Versions: 0.6
> Reporter: Ted Ross
> Assignee: Ted Ross
> Priority: Minor
>
> If you create a binding to the XML exchange (using python/examples/xml-exchange/declare_queues.py for example), then look at the binding using qpid-tool, the arguments field is {} (an empty map).
> Arguments for headers exchanges can be viewed correctly, this only affects the xml exchange.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
[jira] Updated: (QPID-2335) Arguments for a binding to an XML
exchange are not visible through management
Posted by "john dunning (JIRA)" <qp...@incubator.apache.org>.
[ https://issues.apache.org/jira/browse/QPID-2335?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
john dunning updated QPID-2335:
-------------------------------
Attachment: qpid-2335.diff
Proposed fix.
I'm no longer able to reproduce the race condition that I saw before, so I'm treating that as a separate issue. This fixes the main problem we started with, plus what looked like an ordering problem when building the binding, as well as a bug I found in the test suite while testing it.
> Arguments for a binding to an XML exchange are not visible through management
> -----------------------------------------------------------------------------
>
> Key: QPID-2335
> URL: https://issues.apache.org/jira/browse/QPID-2335
> Project: Qpid
> Issue Type: Bug
> Components: C++ Broker
> Affects Versions: 0.6
> Reporter: Ted Ross
> Assignee: Ted Ross
> Priority: Minor
> Attachments: qpid-2335.diff
>
>
> If you create a binding to the XML exchange (using python/examples/xml-exchange/declare_queues.py for example), then look at the binding using qpid-tool, the arguments field is {} (an empty map).
> Arguments for headers exchanges can be viewed correctly, this only affects the xml exchange.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
[jira] Commented: (QPID-2335) Arguments for a binding to an XML
exchange are not visible through management
Posted by "Gordon Sim (JIRA)" <qp...@incubator.apache.org>.
[ https://issues.apache.org/jira/browse/QPID-2335?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12800364#action_12800364 ]
Gordon Sim commented on QPID-2335:
----------------------------------
I don't think it is gratuitous to have a subclass add an argument to its constructor to initialise a member it defines. To me that seems entirely sensible.
The Exchange::Binding instance is not stashed in a global location - the addObject() method only passes in the mgmtBinding pointer - but it will be accessed by multiple threads (the exchanges are required to be threadsafe). The only mutable members of Exchange::Binding are the mgmtBinding and the queue pointer. The queue pointer can be marked const leaving just the mgmtBinding pointer which can then be made private. I think that goes a reasonable way to preventing incorrect usage. Would that alleviate your concerns?
> Arguments for a binding to an XML exchange are not visible through management
> -----------------------------------------------------------------------------
>
> Key: QPID-2335
> URL: https://issues.apache.org/jira/browse/QPID-2335
> Project: Qpid
> Issue Type: Bug
> Components: C++ Broker
> Affects Versions: 0.6
> Reporter: Ted Ross
> Assignee: Ted Ross
> Priority: Minor
>
> If you create a binding to the XML exchange (using python/examples/xml-exchange/declare_queues.py for example), then look at the binding using qpid-tool, the arguments field is {} (an empty map).
> Arguments for headers exchanges can be viewed correctly, this only affects the xml exchange.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
[jira] Commented: (QPID-2335) Arguments for a binding to an XML
exchange are not visible through management
Posted by "Gordon Sim (JIRA)" <qp...@incubator.apache.org>.
[ https://issues.apache.org/jira/browse/QPID-2335?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12800298#action_12800298 ]
Gordon Sim commented on QPID-2335:
----------------------------------
What about:
Index: cpp/src/qpid/xml/XmlExchange.cpp
===================================================================
--- cpp/src/qpid/xml/XmlExchange.cpp (revision 898714)
+++ cpp/src/qpid/xml/XmlExchange.cpp (working copy)
@@ -82,7 +82,7 @@
XmlBinding::vector::ConstPtr p = bindings.snapshot();
if (!p || std::find_if(p->begin(), p->end(), MatchQueue(queue)) == p->end()) {
Query query(xqilla.parse(X(queryText.c_str())));
- XmlBinding::shared_ptr binding(new XmlBinding (routingKey, queue, this, query));
+ XmlBinding::shared_ptr binding(new XmlBinding (routingKey, queue, this, *bindingArguments, query));
bindings.add(binding);
QPID_LOG(trace, "Bound successfully with query: " << queryText );
Index: cpp/src/qpid/xml/XmlExchange.h
===================================================================
--- cpp/src/qpid/xml/XmlExchange.h (revision 898714)
+++ cpp/src/qpid/xml/XmlExchange.h (working copy)
@@ -49,8 +49,8 @@
boost::shared_ptr<XQQuery> xquery;
bool parse_message_content;
- XmlBinding(const std::string& key, const Queue::shared_ptr queue, Exchange* parent, Query query):
- Binding(key, queue, parent), xquery(query), parse_message_content(true) {}
+ XmlBinding(const std::string& key, const Queue::shared_ptr queue, Exchange* parent, const framing::FieldTable& args, Query query):
+ Binding(key, queue, parent, args), xquery(query), parse_message_content(true) {}
};
Your concern around potential concurrent access to Exchange::Binding::mgmtBinding seems like it would be better addressed via a separate Jira? My inclination (though I'm not sure I properly understand your concern) would be something simple like making that member private:
Index: cpp/src/qpid/broker/Exchange.cpp
===================================================================
--- cpp/src/qpid/broker/Exchange.cpp (revision 898714)
+++ cpp/src/qpid/broker/Exchange.cpp (working copy)
@@ -87,9 +87,7 @@
msg.getMessage().blockContentRelease();
for(std::vector<Binding::shared_ptr>::const_iterator i = b->begin(); i != b->end(); i++, count++) {
- msg.deliverTo((*i)->queue);
- if ((*i)->mgmtBinding != 0)
- (*i)->mgmtBinding->inc_msgMatched();
+ (*i)->route(msg);
}
}
@@ -333,6 +331,13 @@
}
}
+void Exchange::Binding::route(Deliverable& msg)
+{
+ msg.deliverTo(queue);
+ if (mgmtBinding != 0)
+ mgmtBinding->inc_msgMatched();
+}
+
Exchange::Binding::~Binding ()
{
if (mgmtBinding != 0) {
Index: cpp/src/qpid/broker/Exchange.h
===================================================================
--- cpp/src/qpid/broker/Exchange.h (revision 898714)
+++ cpp/src/qpid/broker/Exchange.h (working copy)
@@ -41,19 +41,22 @@
class Exchange : public PersistableExchange, public management::Manageable {
public:
- struct Binding : public management::Manageable {
+ class Binding : public management::Manageable {
+ public:
typedef boost::shared_ptr<Binding> shared_ptr;
typedef std::vector<Binding::shared_ptr> vector;
Queue::shared_ptr queue;
const std::string key;
const framing::FieldTable args;
- qmf::org::apache::qpid::broker::Binding* mgmtBinding;
Binding(const std::string& key, Queue::shared_ptr queue, Exchange* parent = 0,
framing::FieldTable args = framing::FieldTable(), const std::string& origin = std::string());
~Binding();
management::ManagementObject* GetManagementObject() const;
+ void route(Deliverable& msg);
+ private:
+ qmf::org::apache::qpid::broker::Binding* mgmtBinding;
};
private:
> Arguments for a binding to an XML exchange are not visible through management
> -----------------------------------------------------------------------------
>
> Key: QPID-2335
> URL: https://issues.apache.org/jira/browse/QPID-2335
> Project: Qpid
> Issue Type: Bug
> Components: C++ Broker
> Affects Versions: 0.6
> Reporter: Ted Ross
> Assignee: Ted Ross
> Priority: Minor
>
> If you create a binding to the XML exchange (using python/examples/xml-exchange/declare_queues.py for example), then look at the binding using qpid-tool, the arguments field is {} (an empty map).
> Arguments for headers exchanges can be viewed correctly, this only affects the xml exchange.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org