search

arobson / rabbot

Deprecated: Please see https://github.com/Foo-Foo-MQ/foo-foo-mq
MIT License
277 stars 129 forks source link

Fix memory leak by disabling routing key caching #62

Closed luddd3 closed 7 years ago

luddd3 commented 7 years ago

By default the postal routing key cache is never cleared. That is effectively a memory leak when a large number of routing keys is used.

arobson commented 7 years ago

@luddd3 - have you measured the impact this has on consumer throughput behavior? You'd need a pretty massive or unbounded number of routing keys before this became a memory leak but removing the caching behavior will have a real and immediate impact on how quickly every single message is resolved to the handler. Without an understanding of consumer limits, this could lead to a lot of confusion on the part of folks who are unfamiliar with the guts of Rabbit/AMQP.

I would like to make this an optional configuration for now and allow users to opt into this new behavior. If, over time, everyone was reporting that this is what they use because the perf hit is negligible and they found they were alway shitting memory issues, we could change the default to this.

luddd3 commented 7 years ago

@arobson as you say, I have a topic queue with a "star binding" (foo.*.bar) where the star is unbound. I have been using this fix for the past month, and haven't had any issues since then.

Another organization (ErisExchange/crugg) did also run into the memory leak issue about the same time as I did. They published a pull request to postal https://github.com/postaljs/postal.js/pull/164 which was merged a while ago, hence version 1.0.5.

For me, the performance inpact has been neglible, but I understand that might not be the case for everyone. When I was digging through issues related to this in postals github repo, I found the following discussion, which I think is similar to the one that we are having right now. It actually references some of your work =) https://github.com/postaljs/postal.js/issues/95

luddd3 commented 7 years ago

@arobson I have turned it into the option 'noCacheKeys'