Re: [PR] Fixed race conditions with ActiveMQTextMessage [activemq]

2026-04-01 Thread via GitHub


cshannon commented on PR #1851:
URL: https://github.com/apache/activemq/pull/1851#issuecomment-4169811718

   @arnoudja - The order of the two calls doesn't matter at all as they do 
different things. The onSend() call marks the messages as read only and 
prepares bytes messages (for example) by closing the output stream and storing 
in the right format. The beforeMarshal() is used for handling any conversion 
work needed to send over the wire that might not be done like converting text 
to bytes. Stomp has the same order, but in reality it doesn't really matter.
   
   The usage is confusing, really there should just be one API call that does 
all of that and that is something to look into with any refactor. For the VM 
transport it's the same, I think either order works as long as it's done before 
the message is actually copied or sent. VM transport essentially always needs 
to have messages copied for this reason. In the broker the VMtransport always 
copies before dispatching to consumers.
   
   In terms of how the refactoring would go on the server side I will just have 
to prototype something. Until I start playing with it I won't really know how 
it will play out. I am going to close this issue out for now because the other 
PR fixed the AMQP issue. I can create a new issue for the refactoring 
investigation


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact




Re: [PR] Fixed race conditions with ActiveMQTextMessage [activemq]

2026-04-01 Thread via GitHub


cshannon closed pull request #1851: Fixed race conditions with 
ActiveMQTextMessage
URL: https://github.com/apache/activemq/pull/1851


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact




Re: [PR] Fixed race conditions with ActiveMQTextMessage [activemq]

2026-03-31 Thread via GitHub


arnoudja commented on PR #1851:
URL: https://github.com/apache/activemq/pull/1851#issuecomment-4165010468

   @cshannon Thanks for the fix! One check, are you sure the call to 
beforeMarshall should be done after calling onSend and not before?
   
   As for the STOMP part: I've looked into it, and apparently we're not using 
STOMP for this but Ajax over HTTP. Sorry for the confusion. Looks like I can 
reproduce this on my machine now when using vm://. Looking at that scenario, 
should beforeMarshall also be called in ActiveMQSession.java in method send() 
just after (or before) the msg.onSend() line? That seems to fix it for me, but 
ofcourse I could be overlooking something.
   
   As for your restructuring idea, I think that'll be very useful but 
personally I wouldn't choose to put something like ActiveMQServerTextMessage 
above ActiveMQTextMessage. That would add yet another layer of complexity and 
feels like it's the wrong way around. My first idea would be to rename the 
current ActiveMQTextMessage, maybe to ActiveMQServerTextMesssage, make 
improvements in that one and add a new ActiveMQTextMessage class with the exact 
same interface as a wrapper around the renamed one. But I haven't checked 
whether such an approach would be feasible.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact




Re: [PR] Fixed race conditions with ActiveMQTextMessage [activemq]

2026-03-30 Thread via GitHub


cshannon commented on PR #1851:
URL: https://github.com/apache/activemq/pull/1851#issuecomment-4158426942

   @arnoudja - I took care of AMQP and merged it in in #1859. I talked talked 
to @tabish121 a bit about some of my ideas for making things more thread safe 
and the SoftRefrence idea probably won't work because of the large impact on GC 
(longer GC pauses, fragmentation, etc). I'm going to look into this though and 
my other ideas.
   
   Let me know if you are able to figure out where things are going wrong with 
when you test with Stomp


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact




Re: [PR] Fixed race conditions with ActiveMQTextMessage [activemq]

2026-03-29 Thread via GitHub


cshannon commented on PR #1851:
URL: https://github.com/apache/activemq/pull/1851#issuecomment-4151361840

   > Would be good to have that AMQP fix in, let me know if I can help or you 
want me to create the pr.
   > 
   
   Yeah, we should create a fix first for AMQP. It's a one line fix so I can 
create a PR real quick tomorrow along with a couple tests. This will bring AMQP 
in line with OpenWire/Stomp.
   
   > So far no luck reproducing the STOMP issue on my machine. Let's separate 
that from the AMQP issue. If it's a client side issue, we can solve it 
ourselves and if it turns out to be a bug in ActiveMQ I'll create a new pr.
   
   It's weird that Stomp still has an issue. I noticed that message properties 
are deserialized before copying on dispatch but not the body. This is 
definitely worth investigating more.
   
   > 
   > The more substantial solution sounds like a good idea to me. There might 
be some first steps without much impact. Making getText() in the 
ActiveMQTextMessage class just return the decoded content without storing it 
looks like an option. It would make subsequent calls to getText slower compared 
to the current situation but that can be solved client side by just reusing the 
result. Taking it a step further could remove the text field completely, with 
setText using the storeContent code directly, but the impact of that is a bit 
harder to see for me.
   
   I took a look at going with the immutable solution and it would be a huge 
impact and it would take a substantial refactor to make messages immutable. The 
current messages mix metadata along with the actual message content so we'd 
probably need to separate the two and maybe have a wrapper that has mutable 
thread safe fields along with the message content that is immutable. This is a 
substantial change and I don't think there's any good way to do it without a 
major version update and probably breaking existing plugins.
   
   Indeed a simple solution is to just store the data only in one format which 
I have considered in the past, just remove the text field entirely. In this 
case we'd just only store the data as the marshaled open wire format. If you 
call setText() the message just immediately converts that to binary. Calling 
getText() would convert and return. As noted, the very obvious downside is the 
performance impact of having to do conversions which is why it wasn't done, at 
least not yet. The question becomes, is this better or worse than using sync? I 
think it depends on the use care primarily.
   
   I have two other ideas that are variants of the above idea (removing the 
text field). I just thought of them and haven't explored so I am not sure if 
they will work or if there will be a show stopper but I think both are possible 
improvements:
   
   1. The first idea is to only require the unmarshaled state (ie text field) 
be null on the server. We can keep the current message implements for client 
code the same so there is no impact. I think this could be achieved by simply 
extending the current impls, ie ActiveMQServerTextMessage or something and that 
class could override the parent methods like setText() and getText() to ensure 
the text field goes unused and we only use the binary. Server code should 
rarely need to use the unmarshaled data as the server's job is generally to 
just move the data along from producers to consumers so it's unlikely to be 
much or any performance issue. I think the biggest risk is users with custom 
plugins that might be touching the messages and calling those fields.
   2. Another idea I had was to always keep the data stored as binary and never 
clear it, but we could keep also keep the text field and convert it to a 
[SoftReference](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/ref/SoftReference.html).
 My thinking is the first time getText() is accessed (or if a user sets it 
directly) we can do the conversion and store the String value in the soft 
reference. This would allow the garbage collector to clean it up if running low 
on memory but if not it would stay cached so repeated calls do not have a 
penalty. When calling getText() we would grab a reference to the value and if 
the String is null we would re-run the conversion. There's no risk of 
accidentally returning null data because we wouldn't ever clear the binary like 
we do now when we set the text. I just thought of this idea today and I don't 
know if there's still possible race conditions (probably) and if it would cause 
other issues with memory/GC. But this helps s
 olve the problem of storing the data two times but only accounting for the 
data one time in memory tracking because the GC can just clear that cached text 
field if we need memory and we can recompute.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsu

Re: [PR] Fixed race conditions with ActiveMQTextMessage [activemq]

2026-03-29 Thread via GitHub


arnoudja commented on code in PR #1851:
URL: https://github.com/apache/activemq/pull/1851#discussion_r3005979990


##
activemq-client/src/main/java/org/apache/activemq/command/ActiveMQTextMessage.java:
##
@@ -182,15 +181,22 @@ public void clearBody() throws JMSException {
 
 @Override
 public int getSize() {
+int minimumMessageSize = getMinimumMessageSize();
+ByteSequence content = this.content;
 String text = this.text;
-if (size == 0 && content == null && text != null) {
-size = getMinimumMessageSize();
-if (marshalledProperties != null) {
-size += marshalledProperties.getLength();
-}
+
+size = minimumMessageSize;
+if (marshalledProperties != null) {
+size += marshalledProperties.getLength();
+}
+if (content != null) {
+size += content.getLength();
+}
+if (text != null) {
 size += text.length() * 2;
 }
-return super.getSize();
+
+return size;

Review Comment:
   Thanks for pointing that out



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact




Re: [PR] Fixed race conditions with ActiveMQTextMessage [activemq]

2026-03-29 Thread via GitHub


arnoudja commented on PR #1851:
URL: https://github.com/apache/activemq/pull/1851#issuecomment-4149839694

   Would be good to have that AMQP fix in, let me know if I can help or you 
want me to create the pr.
   
   So far no luck reproducing the STOMP issue on my machine. Let's separate 
that from the AMQP issue. If it's a client side issue, we can solve it 
ourselves and if it turns out to be a bug in ActiveMQ I'll create a new pr.
   
   The more substantial solution sounds like a good idea to me. There might be 
some first steps without much impact. Making getText() in the 
ActiveMQTextMessage class just return the decoded content without storing it 
looks like an option. It would make subsequent calls to getText slower compared 
to the current situation but that can be solved client side by just reusing the 
result. Taking it a step further could remove the text field completely, with 
setText using the storeContent code directly, but the impact of that is a bit 
harder to see for me.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact




Re: [PR] Fixed race conditions with ActiveMQTextMessage [activemq]

2026-03-28 Thread via GitHub


jbonofre commented on code in PR #1851:
URL: https://github.com/apache/activemq/pull/1851#discussion_r3005734140


##
activemq-client/src/main/java/org/apache/activemq/command/ActiveMQTextMessage.java:
##
@@ -182,15 +181,22 @@ public void clearBody() throws JMSException {
 
 @Override
 public int getSize() {
+int minimumMessageSize = getMinimumMessageSize();
+ByteSequence content = this.content;
 String text = this.text;
-if (size == 0 && content == null && text != null) {
-size = getMinimumMessageSize();
-if (marshalledProperties != null) {
-size += marshalledProperties.getLength();
-}
+
+size = minimumMessageSize;
+if (marshalledProperties != null) {
+size += marshalledProperties.getLength();
+}
+if (content != null) {
+size += content.getLength();
+}
+if (text != null) {
 size += text.length() * 2;
 }
-return super.getSize();
+
+return size;

Review Comment:
   You always recalculates size from scratch, while `super.getSize()` (from 
`Message`)  only recalculates when `size < minimumMessageSize || size == 0`. 
   Here, you directly set `this.size` and returns it, completely bypassing 
`super.getSize()`.
   
   This basically means:
   * The size is recalculated on every single call (potentially performance 
regression)
   * When both `text` and `content` are present (which is now always the case 
after `beforeMarshall`), it double-counts the payload (the text characters and 
their serialized byte form). This inflates memory tracking and could cause 
premature flow control or producer blocking.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact




Re: [PR] Fixed race conditions with ActiveMQTextMessage [activemq]

2026-03-28 Thread via GitHub


cshannon commented on PR #1851:
URL: https://github.com/apache/activemq/pull/1851#issuecomment-4148474076

   Ok sounds good, at the very least next week I will create a PR with the AMQP 
fix but it would be good to identify if here's a Stomp area to fix too.
   
   Ultimately something more substantial needs to be done to fix the issue for 
real. I've been thinking about ways to avoid the problem entirely. 
Synchronization on the message types is an option of course, but has 
performance draw backs. Something I want to experiment with would be to have 
immutable versions of the messages.  We already copy all over the place so the 
server could just use immutable copies once they reach the broker internally 
that only store bytes (might even just have one Shared message type) and then 
there's no issue.
   
   The downside is it it would likely require a major version bump and probably 
breaking changes for things like plugins as I'm sure lots of plugins and code 
do mutations like adding headers. You would need easy ways to copy and convert 
between types, maybe shared interfaces etc.  And all the while not wanting to 
break existing clients. I would have to try it to really know the impact but 
it's worth exploring. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact




Re: [PR] Fixed race conditions with ActiveMQTextMessage [activemq]

2026-03-28 Thread via GitHub


arnoudja commented on PR #1851:
URL: https://github.com/apache/activemq/pull/1851#issuecomment-4148440930

   Yes, that patch is already in and it doesn't solve it in our case. So far 
we've focused on the AMQP part though, so I'll look into the STOMP part to see 
whether I can find a good reproduction scenario. Could very well be a client 
side issue.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact




Re: [PR] Fixed race conditions with ActiveMQTextMessage [activemq]

2026-03-28 Thread via GitHub


cshannon commented on PR #1851:
URL: https://github.com/apache/activemq/pull/1851#issuecomment-4148391962

   @arnoudja - I'm glad to hear that fixed it for AMQP. So does this mean you 
are not running this patch 
https://github.com/apache/activemq/commit/57264bf8dc9970d7d808cf24216b6185ceb644e2
 ? 
   
   That patch is basically the same solution for Stomp. I took a look at Stomp 
and during send to consumers a copy is correctly done before touching the body, 
but it looks like the message headers are deserialized without a copy first so 
that might be something we need to fix (by moving the copy earlier).
   
   If you are not running 
https://github.com/apache/activemq/commit/57264bf8dc9970d7d808cf24216b6185ceb644e2
 then that patch should fix it for Stomp as well.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact




Re: [PR] Fixed race conditions with ActiveMQTextMessage [activemq]

2026-03-28 Thread via GitHub


arnoudja commented on PR #1851:
URL: https://github.com/apache/activemq/pull/1851#issuecomment-4148371323

   I've checked that patch and I can confirm that it solves the AMQP problem. 
Nice solution!
   
   As could be expected, the STOMP problem returned, so I switched back to our 
own solution afterwards. Please be aware that we're using an old branch of 
ActiveMQ, so I'm not sure whether the problem still would occur on the master 
branch. I'll have a look to see if I can find a location where it is 
unmarshalled.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact




Re: [PR] Fixed race conditions with ActiveMQTextMessage [activemq]

2026-03-28 Thread via GitHub


cshannon commented on PR #1851:
URL: https://github.com/apache/activemq/pull/1851#issuecomment-4148120891

   I confirmed that AMQP does not marshal the data before passing to the broker 
which is probably the issue. You already found where Stomp does it.
   
   Can you try this patch? 
https://github.com/apache/activemq/commit/6002d18b0f17ec5e3f22ec076f5e3d84ea80f4f5
   
   You mentioned that the issue still happens with Stomp which I find 
surprising because that method does clear the text and marshal everything 
before it's passed as you linked. I need to check the Stomp dispatch/send code 
to see if it is copying messages first, maybe it is unmarshaling back to text 
on dispatch without copying causing a race but not sure yet.
   
   For your case with AMQP and network bridges, when the VM transport 
dispatches it should copy before handing off to a consumer, and also the AMQP 
protocol sender will 
[copy](https://github.com/apache/activemq/blob/70caa1b4ad0ed556d6119cd8254348c63a482085/activemq-amqp/src/main/java/org/apache/activemq/transport/amqp/protocol/AmqpSender.java#L450)
 the message on dispatch as well because it updates the state and transforms 
it. 
   
   So if the message is already marshaled when incoming, this may fix the issue 
you are seeing, at least for AMQP, so it's at least worth trying to see if the 
small one line patch makes your issue go away as a test.
   
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact




Re: [PR] Fixed race conditions with ActiveMQTextMessage [activemq]

2026-03-28 Thread via GitHub


arnoudja commented on PR #1851:
URL: https://github.com/apache/activemq/pull/1851#issuecomment-4147886696

   As a result to your remark "Openwire messages should arrive on the broker 
and be serialized already into bytes":
   
   As far as I can tell, the ActiveMQTextMessage instance in this path is 
created in this method in JMSMappingInboundTransformer.java:
   
   public static ActiveMQTextMessage createTextMessage(String text) {
   ActiveMQTextMessage message = new ActiveMQTextMessage();
   try {
   message.setText(text);
   } catch (MessageNotWriteableException ex) {}
   
   return message;
   }
   
   As you can see, setText is called which will fill the text field, not the 
content field. Adding a call to setContent here might solve the problem but 
this is probably not the best place to do so.
   
   Also, for the STOMP part, does 
https://github.com/apache/activemq/commit/57264bf8dc9970d7d808cf24216b6185ceb644e2
 address this? That wouldn't, however, explain why we still had the problem 
before this ActiveMQMessageText change.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact




Re: [PR] Fixed race conditions with ActiveMQTextMessage [activemq]

2026-03-28 Thread via GitHub


arnoudja commented on PR #1851:
URL: https://github.com/apache/activemq/pull/1851#issuecomment-414370

   Hello Christopher,
   
   Thank you for looking into this. In our case, this change fixes the problem. 
We're currently running a custom build version of ActiveMQ, the memory usage is 
not an issue in our case. But I fully understand that this will cause problems 
for others so let's look for a solution that serves both cases.
   
   As an answer to your question: Synchronisation will solve this problem for 
us, but it won't solve other problems. The problem is that getText basically 
does the reverse of beforeMarshall. So if thread 1 calls beforeMarshall, thread 
2 calls getText right afterwards and then thread 1 resumes, this will lead to 
problems in all of the following code in thread 1 that rely on beforeMarshall 
being called. Basic synchronisation within the ActiveMQTextMessage class won't 
avoid that, you'll need to block the call to getText between beforeMarshall and 
the last piece of code that depends on that.
   
   This doesn't seem to be just a theoretical scenario but seems possible in 
reality as well, though I haven't tried to reproduce it. See the xpath scenario 
described in my original message for more details. That results in my 
conclusion that synchronisation isn't only an expensive problem but also one 
that only solves part of the problems.
   
   As of the AMQP part: This has definitely something to do with the conversion 
between protocols. Most of our code uses Openwire to communicate with the 
broker and we don't see this empty message problem there. However, we do see 
the same problem with messages posted using STOMP instead of AMQP. This fix 
solves most of the problems for the STOMP posts as well (down from ~1% problem 
cases to ~0.1%). The remaining ~0.1% problem is probably an unrelated client 
side issue but I didn't look into that yet.
   
   So either the STOMP conversion has the same problem or the problem occurs in 
a piece of code used by both the AMQP and STOMP conversion.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact




Re: [PR] Fixed race conditions with ActiveMQTextMessage [activemq]

2026-03-27 Thread via GitHub


cshannon commented on PR #1851:
URL: https://github.com/apache/activemq/pull/1851#issuecomment-4146127914

   Also this may have something to do specifically with AMQP because this issue 
doesn't appear to happen with just OpenWire (at least I'e never seen it). I am 
not familiar with the conversion and I'd need to look but maybe the AMQP 
protocol handler on incoming messages are not storing the contents as binary 
and we end up with text being stored as a String which leads to the race 
condition when having to convert later on dispatch over the network bridge and 
consumer.
   
   Openwire messages should arrive on the broker and be serialized already into 
bytes so they shouldn't need to be converted later as the broker is generally 
not calling getText() (unless a custom plugin as logging or something).
   
   So one part of this fix could involve something with the AMQP conversion


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact




Re: [PR] Fixed race conditions with ActiveMQTextMessage [activemq]

2026-03-27 Thread via GitHub


cshannon commented on PR #1851:
URL: https://github.com/apache/activemq/pull/1851#issuecomment-4146024497

   I started looking at this briefly and the optimization is necessary to avoid 
OOM and because wasting twice as much memory is not great when there are a 
large amount of messages in memory. The optimization was added because when 
there are a lot of messages in cache in the broker you can easily have an OOM 
error. The problem is the memory usage tracking isn't aware that the data is 
stored twice so you can blow past the configured memory limits. We could 
account for both copies for memory tracking which would fix OOM but then still 
the issue of wasting a ton of space.
   
   As you pointed out, the general idea is to operate on independent copies so 
the broker is _supposed_ to make copies so 2 threads are not touching the same 
message, which is the 3rd solution you mentioned and has been preferred. So we 
may ultimately want to go the copy option.
   
   Other ideas include creating broker specific versions of the message classes 
that were thread safe but that might be a huge pain to do or maybe we could 
just only store the text as bytes (and never text) but then there is the 
conversion penalty each time you called getText()
   
   @arnoudja - can you better explain what you mean that synchronization 
wouldn't solve the issues? We have avoided it so far of course but it should be 
possible to add a lock internally to prevent two threads interfering. I am 
still hesitant to do this and if we did we'd probably need to add sync to all 
the classes but I am going to explore the option at least when looking at this 
more.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact




Re: [PR] Fixed race conditions with ActiveMQTextMessage [activemq]

2026-03-27 Thread via GitHub


cshannon commented on PR #1851:
URL: https://github.com/apache/activemq/pull/1851#issuecomment-413800

   @arnoudja - Thanks for the PR I will take a look when I get a chance. As you 
saw from #1659, this issue has come up before in the past a few times.
   
   The primary issue is that the message classes are not designed to be thread 
safe, but as you noticed sometimes it appears that are accessed in a non thread 
safe way. There have been reports over the years of it happening, and while 
there are race conditions we could not pinpoint exactly where in the broker so 
it wasn't clear if it was really a broker issue or some client side problem. 
This was the problem with #1659 and why that thread died out, which was the 
race condition could be created in a unit test artificially but there wasn't a 
good explanation of how to reproduce the issue with a real broker so we can see 
why it was broken in the first place.
   
   It looks like your analysis might shed some light on that mystery so that 
will be helpful. Ideally we'd try and fix it so that we didn't need to sync on 
the actual message itself and just handle/sync in the broker where needed but 
it depends I guess as we want to make sure it's correct. I did a quick scan of 
the PR and it doesn't look like you used sync so I'll take a look closer and 
see what you found.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact