Uncaught Exception handling

[OrH]

Hey,

Is there a specific reason for emitting uncaughtException to the process instead of returning as an error in the callback?

https://github.com/thenativeweb/node-cqrs-eventdenormalizer/blob/master/lib/definitions/viewBuilder.js#L94

    try {
      unwrappedDenormFn.call(this, evtData, vm, wrappedCallback.bind(this));
    } catch (e) {
      debug(e);
      process.emit('uncaughtException', e); // Why not do - wrappedCallback.apply(this, [e]);
    }

Thanks!

[goloroden]

Hi @OrH 👋

To be honest, I don't know – the code you mentioned was not originally written by us, so we can only speculate.

Theoretically, I'd say, let's change this. However, we have taken over the code, but do not yet feel confident enough to be able to foresee which changes in other places / repos this might need.

So, it's probably better to defer this a little bit… sorry that I don't have any better news for now 😞

[OrH]

Hey @goloroden 👋

Thanks for the quick response.

I see.
From my knowledge of the code and usage (we use it for a while now), if we do this change it shouldn't cause issues in this repo or others, because it will just return the error to the handler to be handled there as other errors are handled, instead of throwing it "outside the handler".

I of course understand that it would probably be better to defer a bit and validate, I don't want to do a change that will break it for others.
I will send a pull request anyway with a change, so if it's decided that this change is good, it will be ready, and I'll follow up on this 🙏

Tagging @adrai here, maybe he will have the time to take a look and approve 😊

[adrai]

@OrH If I remember correctly, all these process.emit('uncaughtException', e) calls were introduced to "punish" the developer, because this must be some sort of code error and not a runtime error.
In your example there is some problem in your denormFn, i.e. if you would have some error in this function: https://github.com/thenativeweb/node-cqrs-eventdenormalizer/blob/master/test/integration/fixture/set1/person/viewBuilders/personLeaved.js#L12
Here there should never be any error...

if this is not how you would expect it, I would recommend it to change it in a major version... (cc @robinfehr)

Is there any particular problem why this disturbs you now @OrH ?

[OrH]

Hey @adrai ,

Thanks for responding!

I understand the reason to "punish" the developer, and in our case, an error was actually caused because of a developer mistake but we still don't want it to get "outside" the handling and manage it by rejecting or acking the message in the queue and this can only be done by doing the callback and letting the handler get and handle the error (otherwise we don't have a context).

We also use a prefetch to define the number of messages that can get to the service until ack/reject in order to not overload the memory of the service until they are handled, so if we don't reject/ack, the messages get stuck, but this might be just how we use it.

It is a good way to divide between errors like that and BuisnessRuleErrors, but for now, I can't see another way how not to get the service "stuck" in cases like these and our usage.

(This also regarding domain commands handling which does the same if I'm not mistaken)

WDYT?

P.S
Thanks a lot for this libs and effort, it's much appreciated and they are super great! 🤗

[adrai]

As I'm not using it anymore, I'm probably the wrong person to ask 😉

I just know this was introduced on purpose 🤷‍♂️

So the community needs to decide 😁

[OrH]

😄

Got it!

Great luck to you with your other path! 🙏 🚀