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

Improves thread safety of lazy initializations #641

Merged
merged 3 commits into from
Jan 28, 2020

Conversation

scott-ferguson-unity
Copy link
Contributor

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.

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.
Copy link
Owner

@jbevain jbevain left a comment

Choose a reason for hiding this comment

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

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.

Mono.Cecil.Cil/MethodBody.cs Outdated Show resolved Hide resolved
Mono.Cecil.Cil/MethodBody.cs Outdated Show resolved Hide resolved
Mono.Cecil/SecurityDeclaration.cs Outdated Show resolved Hide resolved
Mono.Collections.Generic/ReadOnlyCollection.cs Outdated Show resolved Hide resolved
@jbevain
Copy link
Owner

jbevain commented Jan 24, 2020

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.

Copy link
Contributor Author

@scott-ferguson-unity scott-ferguson-unity left a comment

Choose a reason for hiding this comment

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

Updated the formatting issues. The tests do all pass when I run them locally.

Intialize it to null to match pattern for the other references.
Fixes the case were the lazy initialization would break when
public_key_token was Empty<byte>.Array.
@scott-ferguson-unity
Copy link
Contributor Author

The failing tests on Windows were my fault - AssemblyNameReference.public_key_token would sometimes be Array<byte>.Empty instead of null. I updated so the initialized value is null.

I didn't think that breaks anything and keeps the pattern the same as the other cases.

@jbevain
Copy link
Owner

jbevain commented Jan 27, 2020

@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?

@scott-ferguson-unity
Copy link
Contributor Author

No, I don't believe anything else needs to be added.

@jbevain jbevain merged commit 0a2f294 into jbevain:master Jan 28, 2020
@jbevain
Copy link
Owner

jbevain commented Jan 28, 2020

Thanks for the PR!

@teo-tsirpanis
Copy link
Contributor

teo-tsirpanis commented Jan 31, 2020

@scott-ferguson-unity , there is actually a dedicated Lazy class for that which is thread-safe and does the exact thing. This PR brought unnecessary code duplication.

@ltrzesniewski
Copy link
Contributor

@teo-tsirpanis using Lazy<T> would cause increased GC pressure, which I guess would quickly become a problem given these are hot code paths.

@teo-tsirpanis
Copy link
Contributor

@ltrzesniewski
Copy link
Contributor

That 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).

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.

4 participants