-
Notifications
You must be signed in to change notification settings - Fork 27.4k
fix($http): properly increment/decrement $browser.outstandingRequestCount
#14921
Conversation
…Count` Calling `$http` will not increment `$browser.outstandingRequestCount` until after all (potentially) asynchronous request interceptors have been processed and will decrement it before any (potentially) asynchronous response interceptors have been processed. This can lead to `$browser.notifyWhenNoOutstandingRequests` firing prematurely, which can be problematic in end-to-end tests. This commit fixes this, by synchronizing the increment/decrement operations with `$http`'s internal interceptor promise chain. Fixes angular#13782 Closes angular#13862
…st fails Calling `promise.finally(...)` creates a new promise. If we don't return this promise, the user won't be able to attach an error handler to it and thus won't be able to prevent a potential PUR error. This commit also improves the test coverage for the increment/decrement `outstandingRequestCount` fix. Closes angular#13869
We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm. |
1 similar comment
We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm. |
I can confirm I'm okay with these commits being contributed to Google. |
This LGTM but I wonder if it does count as a breaking change for some e2e tests? |
…st fails Calling `promise.finally(...)` creates a new promise. If we don't return this promise, the user won't be able to attach an error handler to it and thus won't be able to prevent a potential PUR error. This commit also improves the test coverage for the increment/decrement `outstandingRequestCount` fix. Closes #13869 Closes #14921
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix.
What is the current behavior? (You can also link to an open issue here)
Calling
$http
will not increment$browser.outstandingRequestCount
until after all (potentiallyasynchronous) request interceptors have been processed and will decrement it before any (potentially
asynchronous) response interceptors have been processed.
This can lead to
$browser.notifyWhenNoOutstandingRequests
firing prematurely, which can beproblematic in end-to-end tests.
See also #13782.
What is the new behavior (if this is a feature change)?
The increment/decrement operations are synchronized with
$http
's internal interceptor promise chain.The
outstandingRequestCount
will be incremented as soon as$http()
is called and will be decremented after all response interceptors have been processed/fulfilled.Does this PR introduce a breaking change?
No (I don't think so).
Please check if the PR fulfills these requirements
Docs have been added / updated (for bug fixes / features)Other information:
Fixes #13782.
Incorporates #13862 and #13869.
This PR is split up in several commits, so that it is easier to review and easier to selectively "lgtm".
It will be much easier to review commit-by-commit 😄
It includes the following (in order of appearance):
$http
code easier to follow.Possibly Unhandled Rejection
error and more thorough test coverage (mainly ported from fix($http): immediatelly increment $browser'soutstandingRequestCount
#13869).