lovelle commented on a change in pull request #3581: Fix thread safety
violation on ProducerImpl
URL: https://github.com/apache/pulsar/pull/3581#discussion_r256678144
##########
File path:
pulsar-client/src/main/java/org/apache/pulsar/client/impl/ProducerImpl.java
##########
@@ -803,12 +806,12 @@ protected boolean
verifyLocalBufferIsNotCorrupted(OpSendMsg op) {
}
protected static final class OpSendMsg {
- MessageImpl<?> msg;
- List<MessageImpl<?>> msgs;
- ByteBufPair cmd;
- SendCallback callback;
- long sequenceId;
- long createdAt;
+ volatile MessageImpl<?> msg;
Review comment:
> Is it too heavy to convert all fields to `volatile`?
In a general sense no, it should be as cheap as load memory from L1 cache on
avg, however if another thread on another core is writing the volatile field
the cache-line will be invalidated and would require to load memory access from
different cache access, of course this behavior depends heavily on cpu arch.
Since this is done on client side and is safer than a raw variable, the
overhead of this is I think is negligible.
> Have you seen any issues regarding these fields?
Nope, anyway, I will investigate this further and will try to add a test to
reproduce this, but since it depends on very unlucky timing it might be
difficult to reproduce.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
