-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix Settings.InjectTopLevelFallback
race condition
#7721
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
Fix Settings.InjectTopLevelFallback
race condition
#7721
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self-review
while(true) | ||
{ | ||
var oldConfig = _fallbackConfig.Value; | ||
var newConfig = config.SafeWithFallback(oldConfig); | ||
if (_fallbackConfig.CompareAndSet(oldConfig, newConfig)) | ||
break; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix: Do not assign value to naked field directly, this can cause race condition if multiple Akka extensions running in multiple threads tries to start at the same time.
Scenario:
- Time 0
- Extension "A" reads
_fallbackConfig
intoconfigA
- Extension "B" reads
_fallbackConfig
intoconfigB
- Extension "A" reads
- Time 1
- Extension "A" injects "akka.cluster.tools.singleton" into
configA
- Extension "B" injects "akka.persistence.sql" into
configB
- Extension "A" injects "akka.cluster.tools.singleton" into
- Time 2
- Extension "A" writes
configA
into_fallbackConfig
- Extension "A" writes
- Time 3
- Extension "B" writes
configB
into_fallbackConfig
- Extension "B" writes
- Time 4
- Extension "A" reads "akka.cluster.tools.singleton" from
Settings.Config
, getsNullReferenceException
because it got clobbered by Extension "B"
- Extension "A" reads "akka.cluster.tools.singleton" from
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - that was simple. Let's see how the testing system does.
private readonly Config _userConfig; | ||
//internal static readonly Config AkkaDllConfig = ConfigurationFactory.FromResource<Settings>("Akka.Configuration.Pigeon.conf"); | ||
private Config _fallbackConfig; | ||
private readonly AtomicReference<Config> _fallbackConfig; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, this is a much simpler fix than what I was imagining
while(true) | ||
{ | ||
var oldConfig = _fallbackConfig.Value; | ||
var newConfig = config.SafeWithFallback(oldConfig); | ||
if (_fallbackConfig.CompareAndSet(oldConfig, newConfig)) | ||
break; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes #7426
Changes
AtomicReference<T>
to enforce thread safety