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

Adjust broker locking order slightly #604

Merged
merged 1 commit into from
Feb 16, 2016
Merged

Conversation

eapache
Copy link
Contributor

@eapache eapache commented Feb 14, 2016

I was doing an unrelated experiment with a config that generated a warning on
validation, and I ended up flooded with warnings because we now call Open on
every broker returned from the Client, and we were revalidating the config every
single time. One thing led to another and I made several other adjustments in
the same vein.

  • Move the atomic AlreadyConnected check to before the config validation in
    Broker.Open. Config validation is expensive and can log, no point in doing
    it if we don't have to.
  • Move all the atomic.StoreInt32 calls to after any of their related log
    messages. None of these were real problems, but it removes any possibility of
    out-of-order log messages due to unusual race conditions.
  • Remove the double-check for AlreadyConnected after we've taken the full
    lock. Initially I just removed the log message since it's not something we
    need to log, but upon closer inspection I'm pretty sure this is dead code;
    anything that sets opened to 0 also sets conn to nil first, and does so
    while holding the lock.

@wvanbergen

I was doing an unrelated experiment with a config that generated a warning on
validation, and I ended up flooded with warnings because we now call `Open` on
every broker returned from the Client, and we were revalidating the config every
single time. One thing led to another and I made several other adjustments in
the same vein.

- Move the atomic `AlreadyConnected` check to before the config validation in
  `Broker.Open`. Config validation is expensive and can log, no point in doing
  it if we don't have to.
- Move all the `atomic.StoreInt32` calls to *after* any of their related log
  messages. None of these were real problems, but it removes any possibility of
  out-of-order log messages due to unusual race conditions.
- Remove the double-check for `AlreadyConnected` after we've taken the full
  lock. Initially I just removed the log message since it's not something we
  need to log, but upon closer inspection I'm pretty sure this is dead code;
  anything that sets `opened` to 0 also sets `conn` to nil *first*, and does so
  while holding the lock.
@wvanbergen
Copy link
Contributor

👍

eapache added a commit that referenced this pull request Feb 16, 2016
Adjust broker locking order slightly
@eapache eapache merged commit 1663b48 into master Feb 16, 2016
@eapache eapache deleted the broker-locking-tweaks branch February 16, 2016 15:50
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.

2 participants