[rabbitmq-discuss] CommandAssembler

[Ben Hood]

Hey Steve,

Thanks for the heads up. I can understand that you want to keep the internal stuff internal and not have to maintain it for external consumers.

The reason why we were using it was for a test that uses a patched version of the tracer, so basically we had an import from there.

Having said that, I should maybe look at the new version of the tracer to see of we can just re-patch that, which may save us from monkey patching the assembler (or the code that links against it).

BTW I was wondering under what circumstances you were seeing race conditions in this section of the library. Of late, we've seen a few oddities at some loads (ca. 2000 shovel out-ins/sec) but it is difficult to reproduce and we always just assumed it was a bug in the threading of our shovel.

Cheers,

Ben

On Nov 17, 2011, at 12:05 PM, Steve Powell <steve at rabbitmq.com> wrote:

> Hi Ben,
> 
> Yes, it was a conscious decision -- it goes with the concurrency and
> thread-safety changes we introduced.  Previously AMQCommand was not thread-safe.
> This was partly due to its use of the Assembler inner class, which serially
> shared state with the AMQCommand class.  Also, other parts of the implementation
> called AMQCommand.Assembler methods.
> 
> When re-factoring AMQCommand we extracted CommandAssembler and synchronised on
> it.  This allowed us to remove the back reference to AMQCommand from the
> assembler, and encapsulate its state. If we expose the object we have problems
> with thread-safety and deadlocking again; but I guess exposing the class
> wouldn't be a problem.  However, its relationship with the AMQCommand class is
> now different, and the use of Assembler is limited entirely to the AMQCommand
> class in the new code.
> 
> We kept it package private on general principles, and to enforce no leakage into
> the public client interface.  In general I am taking the view that the less
> visible the internals the harder it is for clients to break them.
> 
> Can you explain how you were using Assembler? We would also be interested in how
> the com.rabbitmq.client.impl packages are being used in general. As a matter of
> long-term intent, the impl classes ought only to be used internally. Though I
> know they are pretty well exposed at present.
> 
> Thank you for your feedback.
> Steve Powell
> 
> On 15 Nov 2011, at 12:30, Ben Hood wrote:
>> I was wondering whether there was a conscious design choice to reduce
>> the visibility of the ported CommandAssembler to default package
>> scope?
>> 
>> We were using the old AMQCommand.Assembler which was public in the
>> com.rabbitmq.client.impl namespace.
>> 
>> Cheers,
>> 
>> Ben