Fix: Issue of synchronizing namespaced local memory caches #37
Conversation
|
Looks perfect! |
|
@Julien-R44 could we get a release pretty please? 🙏 Wanted to try this fix, but I'm running into other issue when trying to install from main. |
|
@wodCZ Yup sorry! Released with 1.0.0-beta.10 |
|
Hmm. I might have misunderstood the goal of this PR, but I'm still having issues with namespace synchronization across multiple processes. I've created a minimal reproduction: https://github.com/wodCZ/bentocache-bus-repro |
|
Thanks for the repro dude ! There is indeed an issue going on. Let me check that |
|
Different issue with BinaryEncoding not handling the Clear message correctly. #38 should fix it. |
|
Released with beta.11 |
| l2Driver: this.l2?.namespace(namespace), | ||
| busDriver: this.#busDriver, | ||
| busOptions: this.#busOptions, | ||
| }) |
There was a problem hiding this comment.
@gkachru I believe, there should be 4th argument passed with this.bus, otherwise a new bus is created every time any namespace is accessed.
This can be seen in https://github.com/wodCZ/bentocache-bus-repro, if you run the main.js, you see the following error after 10 logs:
(node:70661) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 reconnecting listeners added to [Commander]. MaxListeners is 10. Use emitter.setMaxListeners() to increase limit
at genericNodeError (node:internal/errors:983:15)
at wrappedFn (node:internal/errors:537:14)
at _addListener (node:events:593:17)
at EventEmitter.addListener (node:events:611:10)
at RedisTransport.onReconnect (file:///Users/wod/sites/bentotest/node_modules/@boringnode/bus/build/src/transports/redis.js:68:26)
at new Bus (file:///Users/wod/sites/bentotest/node_modules/@boringnode/bus/build/src/bus.js:28:38)
at new Bus (file:///Users/wod/sites/bentotest/node_modules/bentocache/build/index.js:588:17)
at #createBus (file:///Users/wod/sites/bentotest/node_modules/bentocache/build/index.js:1098:20)
at new _CacheStack (file:///Users/wod/sites/bentotest/node_modules/bentocache/build/index.js:1076:31)
at _CacheStack.namespace (file:///Users/wod/sites/bentotest/node_modules/bentocache/build/index.js:1102:12)
Thanks for the amazing work otherwise 🙏
There was a problem hiding this comment.
PS: just checked adding the 4th arg locally and the memory leak warning is gone, while both namespace sync issues remain fixed.
There was a problem hiding this comment.
The bus does indeed need to be created every time, as you need to pass in the new namespace localcache instance. It is however generally a memory intensive way as both CacheStack and bus get created.
The event emitter warning can be eliminated by setting up something like const bcProducts = bento.namespace('products') and then using bcProducts.set etc from then on. However, still you would have as many CacheStack and bus instances as there are namespaces, plus one for the original bento instance.
The reason why .clear() still works as you pass in the original bus, is because all buses are listening to the same queue name! So the clear message is received by every instance of the bus, both the namespaced and non-namespaced one, which will clear all items everywhere!
Have noticed this, and I am working on this issue as well.
There was a problem hiding this comment.
Added PR #39 to address the excessive messages by namespacing the bus channel.
Changes to namespaced local memory caches do not get pushed correctly.
When the CacheStack is namespaced, it creates a namespaced version of the l1 drivers, however does not create a new bus with the modified l1 driver. This change fixes that.
Added some test cases as well. Should fix #32