Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure that connecting method is synchronized #308

Merged
merged 1 commit into from
Feb 9, 2018

Conversation

cbarton
Copy link
Contributor

@cbarton cbarton commented Feb 9, 2018

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.

When there are competing publisher threads, there is a chance that the
publisher has not been initialized when Hutch is registered as
connected. 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 michaelklishin changed the title Ensure that connecting to Rabbit is threadsafe Ensure that connecting to Rabbit is synchronized Feb 9, 2018
@michaelklishin michaelklishin changed the title Ensure that connecting to Rabbit is synchronized Ensure that connecting method is synchronized Feb 9, 2018
@michaelklishin michaelklishin merged commit 3789000 into ruby-amqp:master Feb 9, 2018
@michaelklishin
Copy link
Member

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 cbarton deleted the threadsafety branch February 9, 2018 17:36
@cbarton
Copy link
Contributor Author

cbarton commented Feb 9, 2018

@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
Copy link
Member

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
Copy link
Contributor Author

cbarton commented Feb 9, 2018

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants