Skip to content

Fix TextMessage body thread safety#2188

Open
cshannon wants to merge 3 commits into
apache:mainfrom
cshannon:text-thread-safety-fix
Open

Fix TextMessage body thread safety#2188
cshannon wants to merge 3 commits into
apache:mainfrom
cshannon:text-thread-safety-fix

Conversation

@cshannon

@cshannon cshannon commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

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:

  1. This PR only attempts to fix the body for ActiveMQTextMessage. This is a problem for the body of other message types as well (including Map, object message, etc) and separate PRs can be opened to fix those if others agree on this strategy.
  2. A PR will need to be done to ensure that the properties map is synchronized as well because it has the same potential problem where the state can be mutated.

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.
@cshannon

cshannon commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

@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

@cshannon cshannon marked this pull request as draft July 6, 2026 14:30
@cshannon

cshannon commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

I marked this as a draft as I might be reworking this

@cshannon cshannon marked this pull request as ready for review July 6, 2026 17:15
@cshannon

cshannon commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

1 participant