Fix #1522 Ensure extensions and persistence plugins are only registered/created once #1648
Conversation
This reverts commit b8545a7.
|
@Horusiath @Zetanova care to comment on this, Akka.Persistence gurus? |
|
I have a test that's calling SnapshotFor in two threads concurrently. Fails on On Fri, Jan 15, 2016 at 7:58 PM -0800, "Zetanova" notifications@github.com wrote: The combination of AtomicReference TrySet+CompareAndSet(of IImmutableMap)+Get should be thread safe. If there is a race condition then it is likly to be in likly ImmutableTreeMap or unlikly in AtomicReferenece Maybe fix for ImmutableTreeMap: Maybe Fix for AttomicReference: — |
| if(!_extensions.ContainsKey(extension.ExtensionType)) | ||
| { | ||
| _extensions.TryAdd(extension.ExtensionType, new Lazy<object>(() => extension.CreateExtension(this))); | ||
| } |
There was a problem hiding this comment.
This part was the cause for race conditions in the old code.
there is a check if something does not exist, and if so, it adds an extension.. classic race condition semantics.
The new code fixes that.
There was a problem hiding this comment.
this can be the couse. Should that "return extension.Get(this);" not actualy using the result of _extensions.GetOrAdd ?
There was a problem hiding this comment.
yes, it could just do a return on that call.
|
The problems in the old code does not relate to atomic references or immutable lists, it comes down to standard race conditions... The fixes looks good to me. |
Fix #1522 Ensure extensions and persistence plugins are only registered/created once
|
I reviewed this also and agreed with @rogeralsing - better to use a |
|
Just for reference, in @Horusiath's original PR he talked about some of the potential trade-offs of switching to |
Use ConcurrentDictionary.GetOrAdd instead of TryGet + AddOrUpdate