Fix non-atomic volatile update #6606
Conversation
|
/pulsarbot run-failure-checks |
sijie
added
the
type/cleanup
|
/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.
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.
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.