This was reverted by 47b7898 because it failed internal google tests https://sponge.corp.google.com/a1c7ae5d-bbdd-4493-a424-95b95fabfb8c

Since I can't see what the failed internal tests at google are, is there any input required from me to get this sorted?

I see that you just added the PR action: merge label, but the following checks are still failing:
     forbidden label detected: PR action: cleanup
     status "code-review/pullapprove" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

Any updates on this? Without this fix Protractor e2e tests with chained http calls fails. And we can't update our app past Angular 5.2.1

@mhevery , @lyml , I would like to look into this one.

And I believe the current behavior (before the PR) is correct,
the code https://github.com/lyml/chained-http here.

doChainedHttpCalls() {
    this.initCalls();

    this.http
      .get('https://httpbin.org/delay/1')
      .switchMap(() => {
        this.state = State.afterFirst;
        return this.http.get('https://httpbin.org/delay/1');
      })
      .subscribe(() => {
        this.state = State.afterSecond;
      });

    this.zone.runOutsideAngular(() =>
    this.testability.whenStable(() =>
    this.setStable()));
  }

  doChainedTimeout() {
    this.initCalls();

    Observable.timer(1000)
      .switchMap(() => {
        this.state = State.afterFirst;
        return Observable.timer(1000);
      })
      .subscribe(() => {
        this.state = State.afterSecond;
      });
    this.zone.runOutsideAngular(() => this.testability.whenStable(() => this.setStable()));
  }

• chainedHttp
• timer

behave differently , because Observable.timer internally use setInterval which will make ngZone.hasPendingMacrotasks always be true until the observable is completed.
But in chainedHttp case, ngZone.hasPendingMacroTasks will become false when first http finished.
so the chainedHttp sequence will become

1. 1st http call, macroTask scheduled
2. 1st http call finished, ngZone stable
3. 2nd http call scheduled, macroTask scheduled
4. 2nd http call finished, ngZone stable

I think this is the correct behavior, if we need to consider the chained http call as a single unit, we should provide some other helper method instead of change current Testability behavior.

I think chained http calls is a very common practice, that can't be avoided without rewrite of apps and API they use. Rich use of RXJS in Angular apps promotes huge usage of different concatMap/mergeMap etc calls. Don't waiting on them to complete is nonsense. If we think about timers I suppose we should wait for timers to complete, its totally OK. If timers don't complete they should be excluded from test environment.

@quanterion , I think we should provide some other helper method to let ngZone become stable until Observable.subscribe is called. But I don't think we should modify the standard behavior.

@JiaLiPassion As you can see here waitForAngular() in protractor depends on whenStable()
https://github.com/angular/protractor/blob/964baba5eac52452350bf1d29a191558595c5f1b/lib/clientsidescripts.js
So introducing new helper method don't fix e2e test problems in Protractor

And let us think from definition point of view. Is whenStable() should be true when only first wave of pending tasks are finished? Is component really becomes stable after that? Is zone becomes stable if it should immediately run another task? Is view is stable from user point of view if no data is rendered?

From developer point of view you make rich composition of RxJs streams and calls subscribe only once. And It looks weird that whenStable() returns true before subscribe fires and no data is rendered yet.

I do think whenStable() should be a common way to test components with very popular RxJS stream orchestration. May be there should be another function to wait specially for the first wave of microtasks? I.e. drainCurrentMacrotasks that waits only for tasks currently in queue and clearly indicates that it doesn't wait for anything else.

angular/protractor#4582

@JiaLiPassion You are correct in describing why it doesn't work but it isn't an argument for why it shouldn't work.

The only purpose of the testability api is to tell Protractor when there are no longer any pending asynchronous tasks. That is its only reason for existing.

Nothing inside of angular actually consumes Testability so adding a new testability, let's call it Testability2, would then lead to Testability1 not being consumed. It would also lead to Protractor having to update how they communicate with Angular due to an internal Angular bug.

This pull request does not modify NgZone. It modifies how Testability determines that all pending tasks have been completed. This is needed because Testability does not handle the case of chained http calls correctly, something that is clearly unintended.

@lyml , @quanterion , yes, I agree with you guys that test maybe even angular/core should have some better way to handle such case more friendly, and the expect result in #20921 is perfectly making sense and is a common use case.

What I want to discuss is should we consider such case (such as chained http) to be equals to whenStable.

• consider the definition of what is the pending tasks, about task, for ngZone. The tasks include
• microTask, such as promise.then or other microTask scheduled by ngZone which will run before all pending macroTasks.
• macroTask, such as setTimeout, setInterval, XHR.

So for ngZone, it will only manage the microTask and macroTask listed above, when there is no pending microTasks and macroTasks, ngZone will consider itself stable.

• Consider the timing of whenStable. I think design is the whenStable should be emitted when no pending tasks and trigger a tick to do change detection.

so for the case here.

this.http
      .get('https://httpbin.org/delay/1')
      .switchMap(() => {
        this.state = State.afterFirst;
        return this.http.get('https://httpbin.org/delay/1');
      })
      .subscribe(() => {
        this.state = State.afterSecond;
  });

The sequence is

• 1st http call, macroTask scheduled
• 1st http call finished, ngZone stable
• 2nd http call scheduled, macroTask scheduled
• 2nd http call finished, ngZone stable

it is really ngZone become stable twice not once.

So I think maybe we should have some other status such as pending observables which will wait for macroTasks.

@JiaLiPassion Your considerations makes a lot of sense but I think sequence is not correct.

public click() {
  this.http
    .get('https://httpbin.org/delay/1')
    .switchMap(() => {
      this.state = State.afterFirst;
      return this.http.get('https://httpbin.org/delay/1');
    })
    .subscribe(() => {
      this.state = State.afterSecond;
  });
}

The sequence is

• Button clicked
• 1st http call, macroTask scheduled, ngZone stable
• 1st http call finished, ngZone stable
• 2nd http call scheduled, macroTask scheduled
• 2nd http call finished, ngZone stable

From this point of view the zone is really stable not twice, but three times. There should be no difference between 1st http call sheduled and 2nd http call sheduled

@JiaLiPassion Nobody is talking about modifying NgZone. It is the Testability implementation that is being fixed. There are two different APIs here which has a stable event.

NgZone.onStable an observable which should fire if there are no pending microtasks (does not care about macrotasks at all).

Testability.whenStable which takes a callback that should fire when there are no more pending asynchronous tasks. NgZone.onStable will fire multiple times before Testability.whenStable fires.

In the current implementation Testability.whenStable schedules a microtask after each NgZone.onStable event, it uses this microtask to check if all pending macro tasks are completed and in that case it incorrectly marks the environment as stable. However, for pending http requests it is not enough to wait for a microtask.

Therefore the testability implementation is not working properly (which leads to the bugs reported in Protractor).

@lyml ,

NgZone.onStable an observable which should fire if there are any pending microtasks (does not care about macrotasks at all).

Testability.whenStable which takes a callback that should fire when there are no more pending asynchronous tasks. NgZone.onStable will fire multiple times before Testability.whenStable fires.

Thank you for explanation, you are right, I mixed up the NgZone.onStable with zone.js onHasTask.
I understand your point, and I know your PR is about Testability.whenStable.

So I may misunderstanding the issue.

in my understanding, with the code

this.http
      .get('https://httpbin.org/delay/1')
      .switchMap(() => {
        console.log('after first isStable', this.zone.hasPendingMacrotasks, this.zone.hasPendingMicrotasks);
        this.state = State.afterFirst;
        return this.http.get('https://httpbin.org/delay/1');
      })
      .subscribe(() => {
        console.log('after second isStable', this.zone.hasPendingMacrotasks, this.zone.hasPendingMicrotasks);
        this.state = State.afterSecond;
  });

So about this case, after first http call, there is no pending macroTasks or microTasks, in current Testability._runCallbacksIfReady implementation, it will check isStable() first, then schedule a microTask to trigger whenStable, at that point , I think there is no macroTask to wait.

And the issue this PR want to resolve is the microTask which scheduled by Testability._runCallbacksIfReady will trigger an additional whenStable callback?

Any updates on this issue?

Hi @lyml! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

@lyml , I believe for this case, the underlying reason of the chained http trigger multiple zone stable call is not Testability, the real reason is the way that how zone.js patch xhr. the detailed information can be found here, angular/zone.js#1055.

currently zone.js patch xhr as a macroTask and after xhr.send, zone.js will add a readystatechange listener, when readystate=4, the macroTask will be invoked.

this logic work fine, but because xhr can also add onload or addEventListener('load', listener).
which is exactly @angular/common/http does, https://github.com/angular/angular/blob/master/packages/common/http/src/xhr.ts#L302
and the load event handler will run after readystatechange, so in the load event handler, the > zone may have been stabled, and cause some issues. in this case, the 2nd http.get is run in load > event handler, so the zone has already stabled, so the zone.onStable was triggered twice.

So in the zone.js PR 1055, I just make sure all onload is finished, then finally invoked the internal readystatechange task.

I have created a fork based on your repo, https://github.com/JiaLiPassion/chained-http, with my zone.js fix, everything work fine, I will add more test cases to verify my idea.

@mhevery, please review, thank you!

@JiaLiPassion - do your fixes to zone.js mean that this PR is no longer needed?

@petebacondarwin , yes, this has been fixed in the zone.js repo, so this one can be closed.

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}