search

moquette-io / moquette

Java MQTT lightweight broker
http://moquette-io.github.io/moquette/
Apache License 2.0
2.27k stars 814 forks source link

Reshape QueueRepository API #692

Closed andsel closed 1 year ago

andsel commented 1 year ago

What it does?

Changes signature and name IQueueRepository service to be more fine grained, avoid the full retrieval of queue to process externally.

hylkevds commented 1 year ago

Did you have a specific reason for removing the removeQueue method? It's rather important for not leaking resources (#608)

andsel commented 1 year ago

Hi @hylkevds removeQueue was actually an used method. So I would be useful if we identify the correct point where it has to be invoked, so in the mean time it's useless to keep the code till I have a right place where it has to be called. It could happen that once I identify the point where the remove it's needed that method has completely another shape, for example "remove queues older than x days" instead of just plain queue removal.

hylkevds commented 1 year ago

Yeah, remove not being used is the reason Moquette leaks memory and #608 exists. My prototype in #648 uses the remove method.

andsel commented 1 year ago

Ahh ok. But at this point I don't understand why with #648 the removeQueue is defined in 2 places:

but never invoked. So it seems like the method is defined but never used.

hylkevds commented 1 year ago

Hmm, indeed. That's odd. I think a call got lost somewhere along the way...

andsel commented 1 year ago

If you can, please create a PR to fix it :pray:

hylkevds commented 1 year ago

Yeah, the Session is removed from the pool, the session is cleaned (in Session.cleanup()), and all messages from the session queues are deleted, but the queue itself is not removed. And since the queue is empty by this point, I never noticed, since it takes almost no resources... But that's a bug :)