🐞[BUG]: ErrorHandler triggered twice on one action

[piernik]

I'm submitting a...

[ ] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report  
[ ] Performance issue
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => https://github.com/ngxs/store/blob/master/CONTRIBUTING.md
[ ] Other... Please describe:

Current behavior

I have action that retrives data from backend and throws error.
I run action like this:

this.obiektyActions.change(data)
          .subscribe(() => {
             this.changed.emit(id);
        });

Action itself is something like this:

return this.apiService.change(action.data)
      .pipe(
        catchError((err) => {
// ...
          return throwError(err);
        }),
        tap(resp => {
          // ...
          this.patchCurrentObiekt(ctx, action.obiektId, val);
        })
      );

I have my custom ErrorHandler and it is invoked twice. As I read it's because observable has two or more subscribers: https://stackoverflow.com/questions/47898101/why-my-global-error-handler-get-invoked-twice-in-my-angular-app

I suppose You are subscribing to action, so I get two subscriptions (and ErrorHandler is triggered twice).

If I change action execution to:

this.obiektyActions.change(data)
          .pipe(tap(() => {
             this.changed.emit(id);
        }));

Action is still triggered (so You have to subscribe) but on success this.changed.emit(id); is never triggered.

Edit:
this.obiektyActions.change(data) = return this.store.dispatch(new ChangeDataAction(data));

Expected behavior

ErrorHandler should recive error info only once.

Minimal reproduction of the problem with instructions

https://stackblitz.com/edit/ngxs-error-throw?file=src/app/app.component.ts

What is the motivation / use case for changing the behavior?

Environment

Libs:
- @angular/core version: 7.2.2
- @ngxs/store version: 3.3.4


Browser:
- [ ] Chrome (desktop) version XX
- [ ] Chrome (Android) version XX
- [ ] Chrome (iOS) version XX
- [ ] Firefox version XX
- [ ] Safari (desktop) version XX
- [ ] Safari (iOS) version XX
- [ ] IE version XX
- [ ] Edge version XX
 
For Tooling issues:
- Node version: XX  
- Platform:  

Others:

[splincode]

create stackblitz example

[piernik]

I'll try. Is there a example app with ngxs which I can copy on stackblitz?

[splincode]

https://stackblitz.com/edit/ngxs-simple

[piernik]

Here is working example: https://stackblitz.com/edit/ngxs-error-throw?file=src/app/app.component.ts

[baigfaizan95]

Try to catch the error inside pipe.

@Action(FetchAction)
 fetch(ctx: StateContext<ExampleStateModel>): Observable<any> {
   return this.fetchFromApi()
     .pipe(
       tap(resp => {
         console.log('Success');
       }),
       catchError(error => of(console.log(error)))
     );
 }

[piernik]

But now it will never get to ErrorHandler and it should, but only once.

[baigfaizan95]

I think the errors are emitted from two functions one from the subscribe method inside app.component and other from the action

this.store.dispatch(new FetchAction())
    .subscribe(resp => {
      console.log(resp);
    }, error => {
      console.log(error);
    });

just tried this and i can see the error

[piernik]

@markwhitfeld @amcdnl any comments on this issue?

[cmaart]

Having problems with this as well.

I'd like to have the global error handler as a fallback for when the error is not dealt with in the component.

But the error is triggered twice, once on the subscribe of the dispatch and also on the global error handler. Imo that is actually wrong behavior. The global error handler should only be called for unhandled errors.

I seriously ask myself how others are dealing with error handling + ngxs

[axelboc]

I seriously ask myself how others are dealing with error handling + ngxs

Same feeling. This is the same issue as #781 and #1145.

Personally, I tag errors that are re-thrown from an action handler so I can then swallow them in my global error handler. I basically just append (silence) to the error message and then look for that. Ugly, but better than my error monitoring service getting spammed.

I've also started to do a lot less work in NGXS states than I used to. For instance, I try to call webservices and catch error responses outside of NGXS, which actually makes more sense in most cases. I end up not having to catch errors inside NGXS action handlers very often.

[vmuresanu]

I've also had a damn headache regarding this issue as my global error handler was printing in console 2 times the same error.
First we've called data service directly and saw that actually it was NGXS who was throwing the error the second time. Now we were sure about the potential source of problem. But then we've tried some other places in our code that also were dispatching actions, and it entered our error handler only one time. So something was not right. The difference between first action and second one was that the first one has been subscribed on, and did some post action:
this.comService.dispatchAction(new PatchRules(requestModel)) .subscribe(() => { this.comService.getActiveDialog(AddRuleModalComponent).close(this.data); });
The fix of the problem was:
this.comService.dispatchAction(new PatchRules(requestModel)) .subscribe(() => { this.comService.getActiveDialog(AddRuleModalComponent).close(this.data); }, () => {} );
Providing the error case of the observable (even an empty one).
After we found the fix we tried to understand the root cause of this behavior, and in NGXS documentation https://www.ngxs.io/advanced/errors we found NGXS uses Angular's default ErrorHandler class so if an action throws an error, Angular's ErrorHandler is called. So as the result NGXS calls Angular's ErrorHandler, and we in our code also call our ErrorHandler. Thus it enters the error handler 2 times.

[MatissJanis]

Just tested if this issue is fixed with the latest version: it is not.

Even though in my component I've handled the exception, it still reaches global error handler.

this.store.dispatch(new Login()).subscribe(
  () => {
    console.log('Success');
  },
  () => {
    console.log('Error is handled here');
  },
);

[arturovt]

Just tested if this issue is fixed with the latest version: it is not.

Even though in my component I've handled the exception, it still reaches global error handler.

this.store.dispatch(new Login()).subscribe(
  () => {
    console.log('Success');
  },
  () => {
    console.log('Error is handled here');
  },
);

Originally the issue was not about this.

() => {
    console.log('Error is handled here');
  }

This is not an ErrorHandler.

[MatissJanis]

Fair enough @arturovt

I saw another comment with identical snippet in this thread and assumed this issue would cover it as well (#803 (comment)).

Any suggestions how to move error handling into components? Since is many cases it doesn't make sense to globally handle errors in actions, but it makes sense to handle them on a case-by-case basis in the consumer component.

[arturovt]

@MatissJanis

Trust me, I really understand your case and what you'd want to achieve. This logic (related to the ErrorHandler) has been the cornerstone from the start, so removing it would cause breaking changes and probably further defects or issues.

Since we are convinced that we cannot remove this logic then we could try to find some trade-offs. I remember that my solution was just to not pay attention to those errors from the ErrorHandler 🤣 (since original Angular's ErrorHandler just invokes console.error and nothing more)

[MatissJanis]

@arturovt perhaps this is something to consider for v4? 😃

My current logic is to suppress every HttpErrorResponse that's has a status code of >= 400 and < 500. Not great.. as the dev building the consumer component needs to remember to build error handling.. and if he forgets - global error handler will suppress it and we will never know that it's not handled properly.

But at least that works for now. :)

[arturovt]

@MatissJanis y, you're right, that could be a plan for v4.

[fikkatra]

Why is this issue closed? It seems like there's no fix and no workaround?

[fikkatra]

For those of you, like me, still running into the issue of errors being processed by the ErrorHandler even when they are caught, or errors that are processed twice by the ErrorHandler when they are not caught, we found a solution to bypass this behaviour.

To summarize:

Desired behaviour:

• errors that aren't caught within the action handler, but are caught later (e.g. by the dispatcher), should not be handled by ErrorHandler
• errors that aren't caught anywhere and bubble up, should be handled by ErrorHandler, but only once

Observed behaviour:

• errors that aren't caught within the action handler, but are caught later (e.g. by the dispatcher), ARE handled by ErrorHandler
• errors that aren't caught anywhere and bubble up, are handled by ErrorHandler, but they are handled TWICE (once because NGXS calls ErrorHandler, once because Angular calls ErrorHandler)

Solution:
Unfortunately, NGXS ALWAYS calls ErrorHandler when an error leaves the action handler, there's no way to prevent this (see this comment). The only thing you can do, is customize the logic in ErrorHandler so it can detect whether it is called by ngxs (rather than by Angular for uncaught errors), and have a duplicate detection system to ignore errors.

To do this, write an NgxsPlugin that hooks into the ngxs pipe:

import { Injectable } from '@angular/core';
import { catchError } from 'rxjs/operators';
import { NgxsPlugin } from '@ngxs/store';
import { ErrorConstants } from './models/error-constants';

@Injectable()
export class NgxsErrorPlugin implements NgxsPlugin {
  public handle(state: any, action: any, next: any): void {
    return next(state, action).pipe(
      catchError((error) => {
        error['skipErrorFlag'] = true;
        throw error;
      }),
    );
  }
}

Write a custom ErrorHandler:

@Injectable({
  providedIn: 'root',
})
export class CustomErrorHandler implements ErrorHandler {
  constructor(private customLogger: Logger) {}

  public handleError(error: any): void {
    if (error['skipErrorFlag'] && !error['errorSkippedFlag']) {
      error['errorSkippedFlag'] = true;
      // ignore error
      return;
    }

    this.customLogger.error(error);
    this.handleUncaughtError(error);
  }

  private handleUncaughtError(err: any): void {
    // implement logic for uncaught errors
  }
}

Provide both the plugin and the custom ErrorHandler:

import { ErrorHandler, NgModule } from '@angular/core';
import { NGXS_PLUGINS } from '@ngxs/store';
import { CustomErrorHandler } from './services/custom-error-handler';
import { NgxsErrorPlugin } from './ngxs-errors-plugin';

@NgModule({
  // imports, exports, declarations...
  providers: [
    { provide: ErrorHandler, useClass: CustomErrorHandler },
    {
      provide: NGXS_PLUGINS,
      useClass: NgxsErrorPlugin,
      multi: true,
    },
  ],
})
export class AppModule {}

It's hacky, but at least it enables you to let the dispatcher (rather than the action handler) decide whether it wants to catch/handle the error or not, and have a mechanism to handle errors that aren't caught anywhere.

[TheCelavi]

Same question - why is this closed? Makes no sense, valid reasons are provided for its resolution.

Thanks @fikkatra, we will flag error in similar way.