Improves thread safety of lazy initializations #641
Loading…
Reference in a new issue
No description provided.
Delete branch "improve-thread-safety"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
The Interlocked.Exchange calls will ensure that the same object is
returned from each call even under a race. That's not necessarily
required, but it should also make sure that no write re-ordering issues
occur on platforms that allow it. And it also makes the possibility
of thread safety issues explicit.
Thank you for your contribution. I've only seen a bunch of style issues that need to be addressed but that's otherwise good to go.
@ -53,7 +53,12 @@ namespace Mono.Cecil.Cil {}Nit pick: please add a space before the opening parentheses in
new InstructionCollection (to match the overall codestyle.nit: same.
@ -144,7 +158,7 @@ namespace Mono.Cecil {if (!HasImage || signature == 0)Nit: indent looks wrong (and uses spaces instead of tabs)
@ -19,7 +20,13 @@ namespace Mono.Collections.Generic {static ReadOnlyCollection<T> empty;Nit: should be on the get line.
Also, I'm not quite sure what is up with the failing test on Windows. That might be a by-product of the change, even though it looks unrelated to the PR functionality.
@ -53,7 +53,12 @@ namespace Mono.Cecil.Cil {}updated
@ -53,7 +53,12 @@ namespace Mono.Cecil.Cil {}updated
@ -144,7 +158,7 @@ namespace Mono.Cecil {if (!HasImage || signature == 0)updated
@ -19,7 +20,13 @@ namespace Mono.Collections.Generic {static ReadOnlyCollection<T> empty;updated
Updated the formatting issues. The tests do all pass when I run them locally.
The failing tests on Windows were my fault -
AssemblyNameReference.public_key_tokenwould sometimes beArray<byte>.Emptyinstead of null. I updated so the initialized value isnull.I didn't think that breaks anything and keeps the pattern the same as the other cases.
@scott-ferguson-unity thanks for getting this green. This is good to merge on my end. Do you want to add anything before I merge?
No, I don't believe anything else needs to be added.
Thanks for the PR!
@scott-ferguson-unity , there is actually a dedicated
Lazyclass for that which is thread-safe and does the exact thing. This PR brought unnecessary code duplication.@teo-tsirpanis using
Lazy<T>would cause increased GC pressure, which I guess would quickly become a problem given these are hot code paths.LazyInitializer.EnsureInitializedThat one is much better, but I think it would need to be profiled (it uses either reflection or a lambda, depending on the overload you choose).