-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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 non-atomic volatile update #6606
Conversation
/pulsarbot run-failure-checks |
/pulsarbot run-failure-checks |
/pulsarbot run-failure-checks |
1 similar comment
/pulsarbot run-failure-checks |
What is the motivation for this change? There were different specific reasons for not using volatile in these components. Volatile has a performance penalty and should only be used if strictly needed. Also, it's not a panacea that fixes all concurrency issue. As it is, I think this commit should be reverted. |
@@ -204,7 +207,7 @@ private LongPair removeAtWithoutLock(int index) { | |||
LongPair item = new LongPair(data[index], data[index + 1]); | |||
data[index] = EmptyItem; | |||
data[index + 1] = EmptyItem; | |||
--this.size; |
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.
This is already protected by a write lock. No need to use updater. The volatile is only used to ensure readers are reading the last value of the size.
@@ -1759,7 +1762,7 @@ public boolean isWritable() { | |||
} | |||
|
|||
public void startSendOperation(Producer producer, int msgSize) { | |||
messagePublishBufferSize += msgSize; |
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.
All operations on a connection are happening on same thread
I take back the argument on performance impact.. since this is only adding the static updaters. Though in most cases these updaters are not necessary because the variables are designed to be updated from a single thread or already within the context of a mutex. |
Yeah, this change is more about adding the static updaters. It doesn't change the existing behaviors. The change here is more about making the volatile updates atomic and ensure a consistent pattern across the code. Since it is always a good practice to follow across the whole pulsar code base. Did you see any other problems with this change? |
No description provided.