Skip to content

Conversation

@AlexShvedoff
Copy link
Contributor

…durable topics with autogenerated names

Broker queues defined for Topic consumers of non-durable Topics are assigned auto-generated names that are not visible to JMS clients.

These broker queues are inherently transient and were previously (correctly) unbounded when the connection to the broker was closed. However, because these broker queues are created per consumer, the broker would effectively leak these until the connection was closed.

This change flags such broker queues with the auto-delete flag, enabling the broker to expunge them once the last consumer is closed.

…durable topics with autogenerated names

Broker queues defined for Topic consumers of non-durable Topics are assigned auto-generated names that are not visible to JMS clients.

These broker queues are inherently transient and were previously (correctly) unbounded when the connection to the broker was closed.  However, because these broker queues are created per consumer, the broker would effectively leak these until the connection was closed.

This change flags such broker queues with the auto-delete flag, enabling the broker to expunge them once the last consumer is closed.
@acogoluegnes acogoluegnes self-assigned this Nov 15, 2017
@acogoluegnes acogoluegnes merged commit 798e57b into rabbitmq:master Nov 15, 2017
@acogoluegnes
Copy link
Contributor

@AlexShvedoff Thanks! I was planning to release this in JMS Client 2.0.0, which requires Java 8 or more. Would that work for you?

@claudiobosticco
Copy link

Hello, actually I need it, but for now I'm stuck to Java 7.

acogoluegnes added a commit that referenced this pull request Nov 15, 2017
@acogoluegnes
Copy link
Contributor

@claudiobosticco OK, I'll release this in 1.8.0, along with a Java client update.

@acogoluegnes acogoluegnes added this to the 1.8.0 milestone Nov 15, 2017
acogoluegnes added a commit that referenced this pull request Nov 15, 2017
@AlexShvedoff
Copy link
Contributor Author

Thanks @acogoluegnes!

acogoluegnes added a commit that referenced this pull request Nov 16, 2017
WIP. Doing this by default makes the CTS fail.

References #35
@acogoluegnes
Copy link
Contributor

@AlexShvedoff @claudiobosticco I had to add this as an opt-in, using it as is would make the JMS Compliance Test Suite fail. You'll have to set RMQConnectionFactory#cleanUpServerNamedQueuesForNonDurableTopicsOnSessionClose to true to use it. Hope to release an RC soon.

@AlexShvedoff AlexShvedoff deleted the autoDelete-non-durable-topic-broker-queues branch November 20, 2017 18:14
@AlexShvedoff
Copy link
Contributor Author

I may be misreading how this is structured, but this seems like the JMS compliance test suite may need an update instead. I the case of JMS topic consumers, the use of RMQ broker queues is incidental to how RMQ works.

These are transient "queues" as evidenced by the fact that they have a randomly generated name. Waiting for a connection close to clear them does mean that in some long-running cases it may literally be years before they go away. And there's no way for a JMS client app to clear them manually because the name of the queue is not something that can be gleaned by calling any API.

This all said - you're the expert, and I can be totally misreading how this should work. Ultimately, as long as there is a flag for people to avoid the leak with long-running connections, we have a solution.

@acogoluegnes
Copy link
Contributor

I got your point, I prefer the flag solution right now as we haven't seen anyone complaining for years. We'll make the cleaning the default behavior if it becomes a blocking problem for a majority of users. We released the RC BTW, you can give it a try. Thanks again for reporting this and providing a PR!

@AlexShvedoff
Copy link
Contributor Author

Makes sense. One other thing: it looks like we need plumbing for this new property in RMQObjectProperty otherwise there's no way to plumb this from the JNDI config all the way to where we need it. Sigh.

@michaelklishin
Copy link
Contributor

michaelklishin commented Nov 28, 2017 via email

@acogoluegnes
Copy link
Contributor

@AlexShvedoff @michaelklishin I created an issue to follow up.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants