fix: abort page.waitForRequest/Response when page closes #4865
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yury-s
changed the title
yury-s
changed the title
| @@ -176,9 +176,10 @@ class Helper { | |||
| * @param {!NodeJS.EventEmitter} emitter | |||
| * @param {(string|symbol)} eventName | |||
| * @param {function} predicate | |||
| * @param {!Promise<!Error>} abortPromise | |||
There was a problem hiding this comment.
not you, but this method is missing a JSDoc for timeout. We should really lint for that.
There was a problem hiding this comment.
Done. Added a type annotation for this.
This was referenced Dec 4, 2019
rfojtik
pushed a commit
to rfojtik/puppeteer
that referenced
this pull request
Dec 21, 2019
) We'd like to pass an abortion signal inside Helper.waitForEvent in order to interrupt it when browser/page closes. Several approaches have been considered: 1. Pass CDPSession instance as a another parameter to the helper method and listen to Disconnected event on it. It would introduce undesired dependency on the session object. 2. Listen to the CDPSession closure at the call sites (e.g. waitForRequest) and pass an abortion promise which would be fulfilled when such event is fired. The listeners would have to be removed from the session on successful completion of waitForEvent so we'd have to pass some kind of DisposablePromise which would be disposed during cleanup. Such parameter looked somewhat hairy. 3. Create DisconnectPromise on CDPSession. One potential risk with that is all chained promises would hang around until the event is fired which might inadvertently cause memory leaks. On the other hand, adding such promise to Promise.race will remove dependency as soon as the race is finished. So this is the approach we're taking with one tweak: the promise is created locally inside Page. Ideally the disconnectPromise would throw when the session is closed but it may lead to uncaught promise errors if all chained promises are resolved, to avoid that the promise is resolved with an Error and Helper.waitForEvent throws it later. Fix puppeteer#4733
This was referenced Feb 3, 2020
This was referenced Mar 16, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We'd like to pass an abortion signal inside Helper.waitForEvent in order to interrupt it when browser/page closes. Several approaches have been considered:
Ideally the disconnectPromise would throw when the session is closed but it may lead to uncaught promise errors if all chained promises are resolved, to avoid that the promise is resolved with an Error and Helper.waitForEvent throws it later.
References #4733