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

Rewrite of the AtomicReference #1615

Merged

Conversation

marcpiechura
Copy link
Contributor

Based on the gitter discussion I have rewritten the AtomicReference based on the DotNetty version.

  • it is now restricted to reference types
  • it uses Volatile Read/Write instead of Interlocked.MemoryBarrier for the Value property
  • it has a new method to atomically get the current value and replace it with a new one

@@ -69,6 +69,97 @@ public override void HeartBeat()
{
_status.CompareAndSet(Status.Unknown, Status.Up);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This class has used the old version with a value type, therefore I have copied the old version into this class.

@rogeralsing
Copy link
Contributor

Looks good to me.
cc Mr. Threading @JeffCyr

@JeffCyr
Copy link
Contributor

JeffCyr commented Jan 6, 2016

Looks good for GetAndSet, but CompareAndSet doesn't look thread safe, it should use Interlocked.CompareExchange.

@JeffCyr
Copy link
Contributor

JeffCyr commented Jan 6, 2016

I reviewed the other atomic utils and noted this:

  • AtomicBoolean could be updated to also use Volatile.Read/Write
  • AtomicCounter.Current should use Volatile.Read

@marcpiechura
Copy link
Contributor Author

@JeffCyr The problem is that CompareExchange doesn't return a bool but rather the initial value. So would this be thread safe or is the equal operation not thread safe again?

var result = Interlocked.CompareExchange(ref atomicValue, newValue, expected);

return !Equals(result, newValue); 

Should I add the other classes to this PR as well?

@JeffCyr
Copy link
Contributor

JeffCyr commented Jan 6, 2016

@Silv3rcircl3 It is thread-safe if you compare with the expected original value

var previous = Interlocked.CompareExchange(ref atomicValue, newValue, expected);
return Object.ReferenceEquals(previous, expected);

I think it's safer to compare the reference explicitly with ReferenceEquals. CompareExchange tests the reference here so it's useless to call a possibly overriden Equals method.

@marcpiechura
Copy link
Contributor Author

@JeffCyr alright, thx for the info!

@Aaronontheweb
Copy link
Member

We'll merge this in once @JeffCyr's comments are addressed

@marcpiechura marcpiechura force-pushed the rewrite_of_AtomicReference branch 3 times, most recently from ceea2dc to e36e100 Compare January 8, 2016 07:34
@marcpiechura
Copy link
Contributor Author

@JeffCyr @Aaronontheweb updated.

@rogeralsing
Copy link
Contributor

CoreAPISpec.ApproveCore

cc @Aaronontheweb how do we fix the above?

@marcpiechura
Copy link
Contributor Author

It's working locally, don't know why it doesn't work with the CI server...
Maybe it's this https://github.com/akkadotnet/akka.net/pull/1615/files#diff-0b623d3c0315003b5801b3c3ce2d5a4dL3770 line, but locally it says I need to change it this way.
Could this be some sort of local language settings issue since I have a german windows + keyboard layout?

@rogeralsing
Copy link
Contributor

@Silv3rcircl3 could be, what happens if you revert that line and push it here again?

@marcpiechura marcpiechura force-pushed the rewrite_of_AtomicReference branch from e031685 to dc0f261 Compare January 18, 2016 15:49
@marcpiechura
Copy link
Contributor Author

that's it... will also update my other PR's

@Horusiath
Copy link
Contributor

👍

@Aaronontheweb
Copy link
Member

Going to merge this one in

Aaronontheweb added a commit that referenced this pull request Jan 18, 2016
@Aaronontheweb Aaronontheweb merged commit 339fbf8 into akkadotnet:dev Jan 18, 2016
@marcpiechura marcpiechura deleted the rewrite_of_AtomicReference branch March 4, 2016 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants