Skip to content
This repository has been archived by the owner on May 26, 2022. It is now read-only.

Fix race with reuse of randomness #54

Merged
merged 3 commits into from Apr 23, 2020
Merged

Fix race with reuse of randomness #54

merged 3 commits into from Apr 23, 2020

Conversation

aschmahmann
Copy link
Contributor

There was a logic bug here where it was expected that a StrategyFactory could be used to create multiple instances of a BackoffStrategy. However, each BackoffStrategy created by the factory shared the same instance of rand.Rand which is not thread-safe unless the underlying rand.Source is thread-safe.

Took a stab at a fix here. Given that the interface has to break and that I'd like to fix this asap since it showed up in production #53, here are a few options:

  1. Don't expose the randomness at all in the constructor (works fine, but means we can't necessarily control the seeds in tests)
  2. Put a random seed in the constructor (pretty easy and low effort for developers that don't care about randomness, but easy to misuse if everyone starts passing in 0 instead of rand.Int63() as the seed)
  3. Put a non thread-safe source in the constructor that gets wrapped into a thread-safe Rand per factory. That thread-safe Rand is used in each BackoffStrategy generated from the factory.
  4. Put a non thread-safe source in the constructor that gets wrapped into a thread-safe Rand per factory. Every time the factory is utilized it creates a new Rand using a seed taken from the Factory-shared Rand.

3+4 seem like the most appealing options to me. I've chosen 3 over 4 in this PR because for now I'd prefer to run into lock contention on the Factory then have lots of allocations for new Rand's. I'm definitely up for better suggestions as long as they don't involve a huge refactor since we're a bit time bound here.

@aschmahmann aschmahmann marked this pull request as ready for review April 22, 2020 23:16
Copy link

@petar petar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it looks good. Google styleguide would dictate that its caller's responsibility to give you a thread safe rand.Source (since they know if intend to call you concurrently) and this is part of the doc contract. But what you've done is less error prone.

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

Successfully merging this pull request may close these issues.

None yet

3 participants