-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10155/#review18425
-----------------------------------------------------------
Ship it!
Looks fine - but I'm not a big fan of the xxxLH() convention, though. It's too
brittle. A code change that (re)moves the lock will cause the code to fail,
but cannot be detected by the compiler. Not much of an issue if the developer
is careful, but a real problem when backporting patches.
My preference - while not a very popular one - is to change the parameter list
of the xxxLH() call to take a const reference to the held lock, while the
actual function ignores this parameter. This allows the compiler to enforce
that there is a lock around the xxLH() call, since the lock actually has to be
passed. Should a patch be applied that (re)moves the lock, the compilation
will fail.
Example:
ScopedLock l(somelock);
...
xxxLH( param1, param2, ..., l );
with the definition of xxxLH() ignoring that parameter:
void xxxLH( int param1, int param2, ... const ScopedLock& /*ignored*/ ) { ....
I can see why most people don't like this convention - it implies the method
may modify the lock - but the const designation prevents that.
That said, I'm fine with the patch as-is.
- Kenneth Giusti
On March 27, 2013, 2:38 p.m., Chug Rolke wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10155/
> -----------------------------------------------------------
>
> (Updated March 27, 2013, 2:38 p.m.)
>
>
> Review request for qpid, Alan Conway, Gordon Sim, and Kenneth Giusti.
>
>
> Description
> -------
>
> Self test federation.test_dynamic_topic hangs. Two threads take out an Rlock
> and then try to upgrade the lock to a Wlock and hang.
>
> The failure was intermittent. Only automake builds running 'make check'
> seemed to have the issue.
>
> The fix is to create an unbindLH() function that does the unbind but does not
> take out the Wlock itself.
>
> The note in the Jira about similar behavior in the TopicExchange is wrong;
> this issue is XmlExchange only.
>
>
> This addresses bug QPID-4672.
> https://issues.apache.org/jira/browse/QPID-4672
>
>
> Diffs
> -----
>
> trunk/qpid/cpp/src/qpid/xml/XmlExchange.h 1461592
> trunk/qpid/cpp/src/qpid/xml/XmlExchange.cpp 1461592
>
> Diff: https://reviews.apache.org/r/10155/diff/
>
>
> Testing
> -------
>
> Passes automake and cmake tests.
>
>
> Thanks,
>
> Chug Rolke
>
>
