Silent renew triggers Angular change detection

[lwoehlck-clgx]

Silent renew triggers Angular change detection

To Reproduce
Steps to reproduce the behavior:

1. Configure an IDP that supports refresh tokens
2. Configure an Angular application with silentRenew and useRefreshToken set to true
3. Set logLevel: LogLevel.Debug
4. Log calls to ngDoCheck in app component ngDoCheck() { console.log('ngDoCheck called'); }
5. Open Console and observe corresponding calls between silentRenew and ngDoCheck

Expected behavior
ngDoCheck should not be called due to silent renews

Desktop:

• OS: Windows 10
• Browser: chrome
• Version 93.0.4577.82 64 bit

Additional context
In https://github.com/damienbod/angular-auth-oidc-client/blob/3de72d57b53f479f946d30b9b77a5bfa975aec79/projects/angular-auth-oidc-client/src/lib/iframe/check-session.service.ts, line 147, we are calling zone.runOutsideAngular() and then calling zone.run() inside that. zone.run() is used to run code in the Angular zone (and therefore gain automatic change detection), and is undoing what zone.runOutsideAngular() is accomplishing (which is to run code without triggering change detection). This could be rectified by removing zone.run(). See https://angular.io/guide/zone#ngzone-run-and-runoutsideofangular.

[maechler]

@lwoehlck-clgx Thanks for the nice summary! We are experiencing the same issue with change detection running every 3s.

As you have already described, I think the current pattern does not help to reduce the number of change detection runs. Looking at the commit 23b4b78 that introduced the change, it seemed to be more about ensuring that ApplicationRef.isStable is not affected.

I think removing zone.run completely would break existing applications using checkSessionChanged$ and relying on automatic change detection, although this could be introduced as a breaking change or an optional setting. Alternatively we could try to use zone.run conditionally to reduce the number of change detection runs.

const pollServerSessionRecur = () => {
  // Check stuff here ...
  if (somethingInterestingHappened) {
    zone.run(doSomething);
  }
  setTimeout(pollServerSessionRecur, 3000);
};

zone.runOutsideAngular(pollServerSessionRecur);

There is one more occurrence of that pattern in interval.service.ts, which probably could also be optimized:

angular-auth-oidc-client/projects/angular-auth-oidc-client/src/lib/callback/interval.service.ts

Lines 22 to 24 in 3de72d5

	this.zone.runOutsideAngular(() => {
	intervalId = setInterval(() => this.zone.run(() => subscriber.next()), millisecondsDelayBetweenTokenCheck);
	});

@damienbod What are your thoughts on this? Do you think it would be possible to remove zone.run, make it optional via configuration or use it conditionally to reduce the number of change detection runs?

I am happy to provide additional context if this is related to the specific setup we have, it looks however more like a general issue to me.

[maechler]

I started digging into this a little more and I think we'd really have to address both of these instances to reduce the number of change detection runs:

angular-auth-oidc-client/projects/angular-auth-oidc-client/src/lib/iframe/check-session.service.ts

Lines 226 to 231 in 33372ed

	this.zone.runOutsideAngular(() => {
	this.scheduledHeartBeatRunning = this.document?.defaultView?.setTimeout(
	() => this.zone.run(pollServerSessionRecur),
	this.heartBeatInterval
	) ?? null;
	});

angular-auth-oidc-client/projects/angular-auth-oidc-client/src/lib/callback/interval.service.ts

Lines 22 to 24 in 3de72d5

	this.zone.runOutsideAngular(() => {
	intervalId = setInterval(() => this.zone.run(() => subscriber.next()), millisecondsDelayBetweenTokenCheck);
	});

The current state of my exploration can be found here maechler@6bc32f8. Possible solutions could be:

1. Remove call to zone.run completely

This would break existing applications and is thus probably not feasible.

2. Use zone.run conditionally

For check-session.service.ts we seem to have the two public APIs checkSessionChanged$ and eventService.fireEvent depending on automatic change detection. For checkSessionChanged$ we could implement it in a way, that change detection only gets triggered if there is at least one subscriber:

get checkSessionChanged$(): Observable<boolean> {
  return new Observable((subscriber) => {
    return this.checkSessionChangedInternal$.asObservable().subscribe({
      next: value => this.zone.run(() => subscriber.next(value)),
      error: error => this.zone.run(() => subscriber.next(error)),
      complete: () =>  this.zone.run(() => subscriber.complete()),
    });
  });
}

// ..

this.scheduledHeartBeatRunning = this.document?.defaultView?.setTimeout(
  () => pollServerSessionRecur(),
  this.heartBeatInterval
) ?? null;

// ..

this.zone.runOutsideAngular(() => pollServerSessionRecur());

For eventService.fireEvent we could probably do it similarly for EventTypes.CheckSessionReceived events only, but it gets a little trickier already. I still didn't have a detailed look at interval.service.ts, but it also seems a little tricky to solve that way.

In addition this approach has the drawback that as soon as there is a subscriber, we are back to running change detection every 3s although it's hardly ever necessary. That's why I think we should probably delegate the responsibility of change detection to the consumer for those APIs.

3. Allow to opt-out of zone.run via configuration

After exploring 2) I think this might be the most promising way forward. We could add a setting to conditionally disable the automatic use of zone.run and delegate the responsibility for change detection to the consumer. However it's a little tricky to find all the public API affected by this change and it would need proper documentation to inform the users in what cases they need to run change detection manually.

4. Wait for Angular to provide zoneless change detection

Having all that said, there is already experimental support for zoneless change detection and maybe we could just wait for it to become stable: https://angular.dev/guide/experimental/zoneless

[maechler]

@damienbod @FabianGosebrink What do you think about this?

I still think it would be useful and also feasible to implement my 3. suggestion, adding a config to opt-out of these zone.run calls. I would be willing to prepare a pull request for this if you tell me there is a decent chance that it makes it into the library.

I'm not sure yet if such a config should be part of PassedInitialConfig or OpenIdConfiguration though. I'm leaning towards PassedInitialConfig, as it seems to be something global. If it was OpenIdConfiguration, we also would need to think about how to handle multiple configurations.

[FabianGosebrink]

Hey Markus, thanks for your message. Sure, this PR has an absolute chance to be merged. I am thinking about this config option: Why would somebody opt out? I mean, 99% do NOT want the CD to run I suppose. What do you think?

[maechler]

Agree, I just thought it would be nice not to make it a breaking change. I think simply removing zone.run could potentially break checkSessionChanged$ and eventService subscribers that rely on being in the zone.

But we could also just make it a breaking change or optionally first introduce it with a feature flag and then change the default value with the next major release.

[FabianGosebrink]

I remember that we once had issues with this. WE really have to test this well.