ARTEMIS-5349 Bridges with concurrency can leak #5557
Conversation
...tion-tests/src/test/java/org/apache/activemq/artemis/tests/integration/jms/RedeployTest.java
Outdated
Show resolved
Hide resolved
|
What's leaking?... I would recommend adding a test under ./leak-tests... It is using a pet library I created... you can write code like this: CheckLeak leak = new CheckLeak(); |
AntonRoskvist
force-pushed
the
ARTEMIS-5349
branch
from
6284b89 to
34544d3
Compare
Bridges where leaking, such that on all configuration reloads, concurrency number of new bridges where added each time and the previous ones where left without management. The effect would be an ever increasing amount of consumers connected to the bridged queue that could not be closed, except from a broker restart. |
| @@ -62,7 +62,7 @@ public void decode(ActiveMQBuffer buffer) { | |||
| } | |||
|
|
|||
| public String getName() { | |||
| return bridgeConfiguration.getParentName(); | |||
| return bridgeConfiguration.getName(); | |||
There was a problem hiding this comment.
is this a required change? it seems un-related.
There was a problem hiding this comment.
@clebertsuconic It's not strictly necessary, but without it each configuration reload will log WARN messages (AMQ222124).
I'll go ahead and change it so that the ActiveMQServerImpl#recoverStoredBridges() fetches this value from the underlying bridgeConfig instead and leave the PersistedBridgeConfiguration as it was.
There was a problem hiding this comment.
If needed keep it.. just trying to understand why and if it was not a left over from a debug or something.
There was a problem hiding this comment.
@AntonRoskvist I wasn't trying to prevent a change to stating it was wrong BTW.
AntonRoskvist
force-pushed
the
ARTEMIS-5349
branch
from
34544d3 to
f0986ce
Compare
|
I ran the complete testsuite on this.. and there is a failure here: org.apache.activemq.artemis.tests.integration.persistence.ConfigChangeTest.bridgeConfigChangesPersist |
AntonRoskvist
force-pushed
the
ARTEMIS-5349
branch
from
f0986ce to
3bd74c2
Compare
@clebertsuconic Yeah I don't know how I missed that but thanks for catching it! The tests pass now and I will run the full testsuite in a few hours to really make sure I got it right this time. |
|
@clebertsuconic testsuite looking good |
This affects bridges whose configuration has been persisted. Simply adding persistence to the pre-existing test case captures this bug.