Fix for issue 26 (concurrency in RabbitMqUnitOfWork)

[peuramaki]

RabbitMqUnitOfWork is fixed to support scenario where there are multiple concurrent, distributed transactions. Unit test

When_running_concurrent_units_of_work_and_distributed_tx.Should_launch_all_TransactionCompleted_events() 

reproduces the issue.

[andreasohlund]

Sweet, can you sign our CLA so we can pull this in?

http://particular.net/contributors-license-agreement-consent

[peuramaki]

I've already done that.

[andreasohlund]

Ok, my bad I've must have missed you in the list!

Thanks!

On Mon, Mar 31, 2014 at 12:30 PM, peuramaki notifications@github.comwrote:

I've already done that.

Reply to this email directly or view it on GitHubhttps://github.com//pull/27#issuecomment-39074058
.

[peuramaki]

Any chance of getting v1.2.0 out soon? This is a show stopper for us, but I'd still like to go with official packages.

[peuramaki]

Thread static is not needed with ConcurrentDictionary.

This is the fix, yes. There were two things I fixed in RabbitMqUnitOfWork:

• replaced thread static dictionary with ConcurrentDictionary to make sure that writes in OutStandingOperations are thread safe
• removed OutstandingOperations.Clear(), because it leaked from thread to another. AFAIK, just using ConcurrentDictionary.TryRemove(..) should be the right thing to do anyway.

My understanding of this working correctly now is based on the unit test part of the pull request and the separate test project (https://github.com/peuramaki/NServiceBus.RabbitMq.Issue26)

[andreasohlund]

Thread static is not needed with ConcurrentDictionary.

Not following, we want all calls to bus.Sen|bus.Publish for the same message to add into the same "list" without a thread static you get a new one everytime and will execute:

https://github.com/peuramaki/NServiceBus.RabbitMQ/blob/b3239bd76298e83b5dd730fa0b628b5261136736/src/NServiceBus.RabbitMQ/RabbitMqUnitOfWork.cs#L67

X times instead of 1 right?

replaced thread static dictionary with ConcurrentDictionary to make sure that writes in >OutStandingOperations are thread safe

Hmm, I need to understand this. What other thread is accessing the thread static of the message handling thread?

[peuramaki]

It's

private static ConcurrentDictionary<string, IList<Action<IModel>>> OutstandingOperations ...

so there's just one instance, and thread safety is taken care of by ConcurrentDictionary.

I'm not quite sure what other threads are touching the data, I was hoping to get an answer from you ;-)

The issue only reproduces when I have DTC in play. I'd assume that it affects timing rather than triggers creating more threads though.

You might want to debug through my unit test against the current RabbitMqUnitOfWork to see what happens.

[andreasohlund]

Doh, completely missed the static :)

That said I'm still not thrilled about the concurrent dictionary since now
effectively all operations agains Rabbit will have to be serialized via the
dictionary while that wasn't the case with the thread static.

I need to read up on how the concurrent dictionary works internally to
figure out how much of an issue this might be.

Wonder where the concurrency comes from. I know that the DTC implementation
does the commit on a separate thread. Could that be it?

On Tue, Apr 1, 2014 at 10:45 AM, peuramaki notifications@github.com wrote:

It's

private static ConcurrentDictionary<string, IList<Action>> OutstandingOperations ...

so there's just one instance, and thread safety is taken care of by
ConcurrentDictionary.

I'm not quite sure what other threads are touching the data, I was hoping
to get an answer from you ;-)

The issue only reproduces when I have DTC in play. I'd assume that it
affects timing rather than triggers creating more threads though.

You might want to debug through my unit test against the current
RabbitMqUnitOfWork to see what happens.

Reply to this email directly or view it on GitHubhttps://github.com//pull/27#issuecomment-39182765
.

[peuramaki]

Yep, DTC commit on separate thread could explain the issue.

I'm not quite following you about the serialization thing. Where would there be an extra serialization compared with the original code?

Other than that, concurrent dictionary should do pretty well from performance point of view when there are multiple threads involved.

[andreasohlund]

Ok, I think I got it now, since the transactionCompleted event MAY fire on a separate thread

https://groups.google.com/forum/#!topic/nhibernate-development/PZdMmC29ckY

this means that the actions recorded in the thread static won't be available and we'll not be executing them against Rabbit. This means that we loose them!

The fix is indeed to go with a static. We need to make the cleanup very robust to avoid memory leaks. We'll work on that! Thanks @peuramaki !

[peuramaki]

My pleasure.

When can you let me know when v1.2.0 will be out? Like I said, this is a show stopper for us.

[andreasohlund]

We'll review it and test right away. Expect a release in the next few days

On Tue, Apr 1, 2014 at 1:37 PM, peuramaki notifications@github.com wrote:

My pleasure.

When can you let me know when v1.2.0 will be out? Like I said, this is a
show stopper for us.

Reply to this email directly or view it on GitHubhttps://github.com//pull/27#issuecomment-39195495
.

[peuramaki]

Ok, very good.

I noticed that development branch references NSB core 4.5 alpha something. Should I expect to update NSB as well? We're currently with the latest stable (4.4.2).

[andreasohlund]

We're backwards compat so an upgrade to nsb 4.5 is not required

Sent from my iPhone

On 1 apr 2014, at 17:29, peuramaki notifications@github.com wrote:

Ok, very good.

I noticed that development branch references NSB core 4.5 alpha something. Should I expect to update NSB as well? We're currently with the latest stable (4.4.2).

—
Reply to this email directly or view it on GitHub.

[johnsimons]

Merged in 75aaf87

[johnsimons]

Hi @peuramaki

Release is out now, thank you very much for the patch.