node-cqrs-saga icon indicating copy to clipboard operation
node-cqrs-saga copied to clipboard

currentHandlingRevisions in revisionGuard never gets cleared. Possible memory leak.

Open rakamoviz opened this issue 7 years ago • 5 comments

https://github.com/adrai/node-cqrs-saga/blob/9770440df0e50b5a3897607a07e0252315b25edf/lib/revisionGuard.js#L51

This container (hash) currentHandlingRevisions never seems to get cleared at any point.

I'm concerned about memory leak when I turn on the revisionGuard for my process manager.

Is it a bug? or can you clarify if it's cleared under certain conditions?

thanks.

rakamoviz avatar Mar 19 '19 17:03 rakamoviz

You are right. Can you provide a PR?

adrai avatar Mar 19 '19 22:03 adrai

Any guidance on the expected behavior (e.g.: at which point / conditions element should be taken out from that object?)?

rakamoviz avatar Mar 19 '19 22:03 rakamoviz

Have just looked into the code again... It's just saving a dictionary with the current revisionNumber... should not be a notable memory issue... I would prefer not change it, if there is not an issue with the memory... In theory the elements could be deleted here: https://github.com/adrai/node-cqrs-saga/blob/216765a941965ea35d052b0d5e7a6f45ddb7609e/lib/revisionGuard.js#L214 fyi: it's the same also for cqrs-eventdenormalizer

adrai avatar Mar 20 '19 06:03 adrai

I tried your suggestion (adding the delete item line on the indicated place). Looks like it still works.

But as I examined more the revisionGuard code..., I became less sure, what is the purpose of revisionGuard? I always thought it's like command-bumper (deduplication) in cqrs-domain (to avoid the same event being processed twice by the saga mechanism). But looks like it's not the case.

https://github.com/adrai/node-cqrs-saga/blob/216765a941965ea35d052b0d5e7a6f45ddb7609e/lib/revisionGuard.js#L111

The concatenatedId constructed in getConcatenatedId is only comprised of aggegateId, context, and aggregate name. No reference to the event itself (id, or at least name).

I verified that by modifying the eventId in my request, and I still get rejected by the error "event already handled". (in my scheme, an action button on the web frontend will have the eventId generated previously.... so second and subsequent clicks on the same button will be rejected by the cqrs-saga).

Can you help us understand the purpose and correct use of revisionGuard please. In the meantime, I have to turn it off. However, I think (by disabling it) I'm missing an important feature of this library.

rakamoviz avatar Mar 21 '19 15:03 rakamoviz

I got it. I just re-read the code and experiment with different inputs, and confirmed it is about making sure the PM processes event in order, no skipping (missing) events, etc. And it's understandable that the key (concatenatedId) is only comprised of aggregate infos, because it's about sequence of events that came out of an instance of aggregate (we want to make sure we process events from each aggregate in correct order, and no repeated processing).

rakamoviz avatar Mar 22 '19 16:03 rakamoviz