Skip to content

Conversation

@nparley
Copy link

@nparley nparley commented Jun 19, 2025

Issue: #396

There is a memory leak in get_exchange. This is because each time get_exchange is called with the RobustExchange a new class is created and added to the set. A set is used because it's expected that this will deduplicate the objects. This doesn't work because __eq__ and __hash__ are not defined on the Exchange class. If a user keeps calling this method, it keeps adding to this set, and it can grow very quickly.

There are two ways of fixing this. #678 makes it so we can compare two exchanges, but I think this is not actually expected behavior. If we look at

async def test_get_exchange(self, connection, declare_exchange):
, this test actually does not return the same class; the names are the same, but get_exchange is actually returning a class without auto_delete:

assert <Exchange(test.passive.exchange.A9gV3VUC47nhiktwQ99dV8): auto_delete=True, durable=False, arguments={})> == <Exchange(test.passive.exchange.A9gV3VUC47nhiktwQ99dV8): auto_delete=False, durable=False, arguments={})>

However, it's clear that what is expected to happen here is we return the already declared exchange, which this PR does. There is no way to do this for non robust connections. With these connection, get_exchange is going to create a new class, but it will not fail because of passive=True, so it's sort of cheating as it's not really getting the exchange; it's trying to make a new one and failing as it already exists, therefore the object properties from the get and the declared exchange will not be the same.

I've copied the test across to the robust connection. Here, get_exchange can return the actual object we originally declared, which I think is nicer.

I think this is not breaking because it's not actually possible to declare exchanges with the same name. So, if you had in the past called declare_exchange twice with the same name but different arguments and not passive, you would have gotten an error anyway.

Queues also need unique names but I think this set stuff happened because the server name created for the name might not return or sometimes?

self.name = "<UNNAMED>"

I think it would be better to make unnamed into a unique string so that we could do away with sets for queues as well further simplifying this function

@nparley nparley marked this pull request as ready for review June 19, 2025 19:14
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.

1 participant