[
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:[email protected]
