Fix TextMessage body thread safety#2188
Conversation
This fixes the rare null body issues seen on the broker when using text messages by using synchronization when marshaling or unmarshaling the body into a String to prevent a race condition from causing the body to end up null. This has been optimized to use volatiles and double checked locking to avoid synchronization unless needed.
|
@mattrpav - I might close this out and open up another PR, there's still issues here because of needing to read 2 variables. There's still race conditions where you could read two nulls outside the lock during mutation. I'm going to prototype something with a ReadWriteLock intead to see what it looks like |
|
I marked this as a draft as I might be reworking this |
|
Using a Reentrant lock doesn't make sense because of the memory usage and the added complexity. I went ahead and reworked this a little bit to fix the race condition by ensuring the decision to sync or not is only checked after reading one variable and not two. For example, getText() only relies on whether text is null and will synchronize if it is. This eliminates the race condition of reading text and content and then trying to determine their state outside of a lock when another thread could be mutating both. |
This fixes the rare null body issues seen on the broker when using text messages by using synchronization when marshaling or unmarshaling the body into a String to prevent a race condition from causing the body to end up null. This has been optimized to use volatiles and double checked locking to avoid synchronization unless needed.
For some background, this has been a long running issue that has come up over the years and there have been many proposed ways to fix it (see #1851 and #1659 for examples). Despite the fact that these messages are supposed to be copied and not used across multiple threads there are cases where that is not true and can cause issues (topic consumers on dispatch for example don't copy) so this issue still pops up from time to time.
After looking at all the options and how disruptive it would be, I think this is the best approach for now as a short term fix. I think long term we could look at better solutions. I think ideally on the server side it would be nice to use messages that only store the data in the marshaled format as the vast majority of the the time the body should not need to be unmarshaled anyways (exceptions include protocol conversion or logging). The client versions could still use mutable messages that can store either version.
Work still todo: