search

ruby-amqp / hutch

A system for processing messages from RabbitMQ.
https://gocardless.com/blog/hutch-inter-service-communication-with-rabbitmq/
MIT License
857 stars 137 forks source link

Ensure that connecting method is synchronized #308

Closed cbarton closed 6 years ago

cbarton commented 6 years ago

When there are competing publisher threads, there is a chance that the publisher has not been initialized when Hutch is registered as connected. This happens when connections are lazily created vs at application load (for example). The following diagram should shed some light on the race condition:

        Thread 1                  Hutch                   Thread 2
        --------                  -------                 ---------
t1      Hutch.publish()  --->  connected? (FALSE)
                           |-> open_connection!
t2                             connected? (TRUE)  <--- Hutch.publish()
                               @broker.publish()   |-> NoMethodError (@publisher)
t3                         |-> declare_publisher!
        PUBLISHED        <-|   @broker.publish()

Wrapping the Hutch.connect call should resolve this issue.

michaelklishin commented 6 years ago

Thank you.

There is one thing I'd like to point out. Every Hutch broker also uses at least one channel. Channels are not supposed to be shared between threads for many operations, in particular publishing. This PR does not address that, so application developers need to bring their own synchronization at least for Hutch.publish.

cbarton commented 6 years ago

@michaelklishin I agree that the Hutch.publish calls should be synchronized since the callers might be sharing a channel across threads. I was going to open up a PR for that case after seeing feedback from this PR. Would you be open to that or would you rather leave the publish synchronization up to the application devs?

michaelklishin commented 6 years ago

Synchronization introduced a 10-11% throughput drop (measured a few years ago when the same thing was considered Bunny), less so with Monitor than Mutex.

I'm not sure how many people actually publish concurrently with Hutch, although some may end up doing so without knowing it.

cbarton commented 6 years ago

Ya, I figured the performance overhead would be something to consider in the library making this the default behavior. I'll stand down on the other PR.

Have a good one!