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: [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
