Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How to handle errors thrown from defineCommand / defineEvent handlers? #152

Open
imissyouso opened this issue Jan 30, 2020 · 9 comments
Open

Comments

@imissyouso
Copy link

imissyouso commented Jan 30, 2020

Hi!
For example:

export = domain.defineEvent({},  (data, aggregate) => {
    aggregate.set('balance', data.balance);
    aggregate.set('value', data.value);

    throw new FakeError(); // unhandled, something went wrong or TypeError
});

how to handle such type of errors?

Approach listed below doesn't help.

    async handle(data: object) {
        return new Promise((resolve, reject) => {
            try {
                this.domain.handle(data, (err) => {
                    if (err) {
                        reject(err);
                    } else {
                        resolve();
                    }
                });
            } catch (e) {
                reject(e);
            }
        });
    }

This problem is similar for defineViewBuilder of eventdenormalizer. Can't catch errors there too.

another a little question:
Why custom command handler is so bad?

Command Handler (Be careful!!!)
Is your use case not solvable without a custom command handling? Sagas? Micro-Services?

what if I need to make external operation like 3rd party API http call and generate event with resulted data without saving to eventstore. This event then can be handled by sagas to generate command to update required aggregate.

@adrai
Copy link
Contributor

adrai commented Jan 30, 2020

These type of errors should theoretically be catched via normal, error argument in the handle callback or as uncaughtException, i.e:

process.on('uncaughtException', function (err) {
})

Can you try to update to v2.14.78 ?

@adrai
Copy link
Contributor

adrai commented Jan 30, 2020

Regarding the custom command handler, you are free to use it and to call external operations, etc...
But I would suggest to solve this via a dedicated saga, which will react on an "preparation"-event, do an external api call and generate the "complete"-command

@imissyouso
Copy link
Author

imissyouso commented Jan 31, 2020

These type of errors should theoretically be catched via normal, error argument in the handle callback or as uncaughtException, i.e:

process.on('uncaughtException', function (err) {
})

Can you try to update to v2.14.78 ?

thank you, now it works.
I think we should move list of errors which produces commandRejected events into domain options, because now it is hardcoded in createCommandRejectedEvent of domain.js

    if (err instanceof ValidationError || err instanceof BusinessRuleError ||
        err instanceof AggregateDestroyedError || err instanceof AggregateConcurrencyError ||
        err instanceof DuplicateCommandError) {
      dotty.put(evt, this.definitions.event.payload, {
        command: cmd,
        reason: {
          name: err.name,
          message: err.message,
          more: err.more
        }
      });

      if (!!this.definitions.event.revision && dotty.get(evt, this.definitions.event.revision) === undefined && err.more && err.more.aggregateRevision > -1) {
        dotty.put(evt, this.definitions.event.revision, err.more.aggregateRevision);
      }
    } else {
      evt = null;
    }

in reason of this, if error occurred in commandHandler (for example TypeError), we can't rollback our Saga and as result end user will see the hanged operation. I think we should do rollbacks where it possible even if error was made by programmer on code level.

@adrai
Copy link
Contributor

adrai commented Jan 31, 2020

When Designing this piece of code I discussed this with other persons.
The decision was to always “punish” programmer errors this way, but feel free to suggest a PR.

@imissyouso
Copy link
Author

imissyouso commented Jan 31, 2020

About

process.on('uncaughtException', function (err) {
})

I understand that we can use it in emergency cases if by some reason callback handler was not executed with error object in first argument. But this situation does not allow us to log this error by our OpenTracing server (Jaeger) because to close current trace we need to pass trace id to error handler of Jaeger. In uncaughtException we do not have access to such trace ID because Error object does not contains it itself.

I made this note to explain why we should try to pass error in handler callback in all cases where it possible and leave handling by uncaughtException only for unpredicted errors, for example on library level. This regards all libs not only Domain.

@dmitry-textmagic
Copy link

But then in case of the exception inside command handler, we have to deal with unfinished (not completed nor rolled back) saga and manually rollback actions performed before this exception. On the other hand, imagine that command handler catch any exceptions and thrown them as, let me say, CommandHandlerError and this error type would have listed in the code @imissyouso noted above (with ValidationError, BusinessRuleError etc). Then "commandRejected" event will be produced in a natural way, as it was ValidationError or BusinessRuleError, and the saga would be rolled back.

I understand that having an unhandled exception in command is an unusual situation, but now we have to catch it and do produce "commandRejected" manually, and it looks kinda strange to me because other types of exceptions produce it by themselves.

@imissyouso
Copy link
Author

imissyouso commented Jan 31, 2020

When Designing this piece of code I discussed this with other persons.
The decision was to always “punish” programmer errors this way, but feel free to suggest a PR.

the strange way to punish programmers especially if we are talking about highload systems where in reason of programmer error we will have to rollback all hanged sagas manually - sounds bad for business :)

@adrai
Copy link
Contributor

adrai commented Jan 31, 2020

Always happy for a PR.

@imissyouso
Copy link
Author

imissyouso commented Jan 31, 2020

Always happy for a PR.

currently we are at stage of investigating and collecting potential issues before the real start of development. As soon as we will do it wait PR from us :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants