You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@artemis.apache.org by "jbertram (via GitHub)" <gi...@apache.org> on 2026/02/01 03:58:22 UTC
[PR] ARTEMIS-5874 mitigate dead-lock in STOMP [artemis]
jbertram opened a new pull request, #6207:
URL: https://github.com/apache/artemis/pull/6207
(no comment)
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-5874 mitigate dead-lock in STOMP [artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on PR #6207:
URL: https://github.com/apache/artemis/pull/6207#issuecomment-3835455562
The full test-suite looks good on this.
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-5874 mitigate dead-lock in STOMP [artemis]
Posted by "artnaseef (via GitHub)" <gi...@apache.org>.
artnaseef commented on code in PR #6207:
URL: https://github.com/apache/artemis/pull/6207#discussion_r2755981530
##########
artemis-protocols/artemis-stomp-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/stomp/StompConnection.java:
##########
@@ -81,12 +82,10 @@ public final class StompConnection extends AbstractRemotingConnection {
//this means login is valid. (stomp connection ok)
private boolean valid;
- private boolean destroyed = false;
+ private AtomicBoolean destroyed = new AtomicBoolean(false);
Review Comment:
I see. It does concern me that this makes the code less readable, and therefore less maintainable. Also, for this specific optimization, how valuable can it really be to save the GC of an AtomicBoolean when the lifecycle of that object is tied to the much larger StompConnection object?
I'd love to see measurements that show how much of a gain it provides.
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-5874 mitigate dead-lock in STOMP [artemis]
Posted by "tabish121 (via GitHub)" <gi...@apache.org>.
tabish121 commented on code in PR #6207:
URL: https://github.com/apache/artemis/pull/6207#discussion_r2754851169
##########
artemis-protocols/artemis-stomp-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/stomp/StompConnection.java:
##########
@@ -81,12 +82,10 @@ public final class StompConnection extends AbstractRemotingConnection {
//this means login is valid. (stomp connection ok)
private boolean valid;
- private boolean destroyed = false;
+ private AtomicBoolean destroyed = new AtomicBoolean(false);
Review Comment:
For objects like this where there could be many in existence at the same time or in some use cases can be created and destroyed quickly I tend to prefer an AtomicIntegerFieldUpdater and a volatile int so that each StompConnection object doesn't need to create a new AtomicBoolean. Just a minor thing and up to you if you see value in it for this case.
```
private static final AtomicIntegerFieldUpdater<StompConnection> DESTROYED_UPDATER =
AtomicIntegerFieldUpdater.newUpdater(StompConnection.class, "destroyed");
private volatile int destroyed;
...
if (DESTROYED_UPDATER.compareAndSet(this, 0, 1)) {
....
}
```
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-5874 mitigate dead-lock in STOMP [artemis]
Posted by "artnaseef (via GitHub)" <gi...@apache.org>.
artnaseef commented on code in PR #6207:
URL: https://github.com/apache/artemis/pull/6207#discussion_r2754986620
##########
artemis-protocols/artemis-stomp-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/stomp/StompConnection.java:
##########
@@ -81,12 +82,10 @@ public final class StompConnection extends AbstractRemotingConnection {
//this means login is valid. (stomp connection ok)
private boolean valid;
- private boolean destroyed = false;
+ private AtomicBoolean destroyed = new AtomicBoolean(false);
Review Comment:
What's the benefit of that? Just the small object creation and removal? Also, I recommend strongly to comment it well if using that approach since it is less obvious.
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-5874 mitigate dead-lock in STOMP [artemis]
Posted by "tabish121 (via GitHub)" <gi...@apache.org>.
tabish121 merged PR #6207:
URL: https://github.com/apache/artemis/pull/6207
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-5874 mitigate dead-lock in STOMP [artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on code in PR #6207:
URL: https://github.com/apache/artemis/pull/6207#discussion_r2756727800
##########
artemis-protocols/artemis-stomp-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/stomp/StompConnection.java:
##########
@@ -81,12 +82,10 @@ public final class StompConnection extends AbstractRemotingConnection {
//this means login is valid. (stomp connection ok)
private boolean valid;
- private boolean destroyed = false;
+ private AtomicBoolean destroyed = new AtomicBoolean(false);
Review Comment:
Readability isn't as straight-forward as with an `AtomicBoolean`, but once you see and compartmentalize the redirection it's not bad. Both `AtomicBoolean` and `AtomicIntegerFieldUpdater` are in `java.util.concurrent.atomic` and both use `compareAndSet`. We use patterns like this in over 80 other places across the code-base (not including tests). I don't think it's any less maintainable ultimately.
Regarding GC pressure, there could potentially be many thousands of STOMP connections given that STOMP is a relatively lightweight protocol used by all sorts of devices (small & large). It's worth saving heap where we can.
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-5874 mitigate dead-lock in STOMP [artemis]
Posted by "tabish121 (via GitHub)" <gi...@apache.org>.
tabish121 commented on code in PR #6207:
URL: https://github.com/apache/artemis/pull/6207#discussion_r2754851169
##########
artemis-protocols/artemis-stomp-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/stomp/StompConnection.java:
##########
@@ -81,12 +82,10 @@ public final class StompConnection extends AbstractRemotingConnection {
//this means login is valid. (stomp connection ok)
private boolean valid;
- private boolean destroyed = false;
+ private AtomicBoolean destroyed = new AtomicBoolean(false);
Review Comment:
For objects like this where there could be many in existence at the same time or in some use cases can be created and destroyed quickly I tend to prefer an AtomicIntegerFieldUpdater and a volatile int so that each StompConnection object doesn't need to create a new AtomicBoolean. Just a minor thing and up to you if you see value in it for this case.
```
private static final AtomicIntegerFieldUpdater<StompConnection> DESTROYED_UPDATER =
AtomicIntegerFieldUpdater.newUpdater(StompConnection.class, "destroyed");
private volatile int closed;
...
if (DESTROYED_UPDATER.compareAndSet(this, 0, 1)) {
....
}
```
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-5874 mitigate dead-lock in STOMP [artemis]
Posted by "jbertram (via GitHub)" <gi...@apache.org>.
jbertram commented on code in PR #6207:
URL: https://github.com/apache/artemis/pull/6207#discussion_r2755008921
##########
artemis-protocols/artemis-stomp-protocol/src/main/java/org/apache/activemq/artemis/core/protocol/stomp/StompConnection.java:
##########
@@ -81,12 +82,10 @@ public final class StompConnection extends AbstractRemotingConnection {
//this means login is valid. (stomp connection ok)
private boolean valid;
- private boolean destroyed = false;
+ private AtomicBoolean destroyed = new AtomicBoolean(false);
Review Comment:
@tabish121, I think that's a good suggestion.
The benefit is less GC pressure and it's faster. We use this kind of pattern in many places so it should be clear to folks familiar with the code-base.
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org
Re: [PR] ARTEMIS-5874 mitigate dead-lock in STOMP [artemis]
Posted by "artnaseef (via GitHub)" <gi...@apache.org>.
artnaseef commented on PR #6207:
URL: https://github.com/apache/artemis/pull/6207#issuecomment-3835967580
Confirmed the deadlock is resolved with this fix. Thank you!
--
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: issues-unsubscribe@artemis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@artemis.apache.org
For additional commands, e-mail: issues-help@artemis.apache.org