HTTP request notification #675
Merged
Conversation
This file contains hidden or 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
krichprollsch
approved these changes
May 21, 2025
karlseguin
force-pushed
the
http_request_notifications
branch
2 times, most recently
from
f49d2fe to
ea3319c
Compare
- Add 2 internal notifications 1 - http_request_start 2 - http_request_complete - When Network.enable CDP message is received, browser context registers for these 2 events (when Network.disable is called, it unregisters) - On http_request_start, CDP will emit a Network.requestWillBeSent message. This _does not_ include all the fields, but what we have appears to be enough for puppeteer.waitForNetworkIdle. - On http_request_complete, CDP will emit a Network.responseReceived message. This _does not_ include all the fields, bu what we have appears to be enough for puppeteer.waitForNetworkIdle. We currently don't emit any other new events, including any network-specific lifecycleEvent (i.e. Chrome will emit an networkIdle and networkAlmostIdle). To support this, the following other things were done: - CDP now has a `notification_arena` which is re-used between browser contexts. Normally, CDP code runs based on a "cmd" which has its own message_arena, but these notifications happen out-of-band, so we needed a new arena which is valid for handling 1 notification. - HTTP Client is notification-aware. The SessionState no longer includes the *http.Client directly. It instead includes an http.RequestFactory which is the combination fo the client + a specific configuration (i.e. *Notification). This ensures that all requests made from that factory have the same settings. - However, despite the above, _some_ requests do not appear to emit CDP events, such as loading a <script src="X">. So the page still deals directly with the *http.Client. - Playwright and Puppeteer (but Playwright in particular) are very sensitive to event ordering. These new events have introduced additional sensitivity. The result sent to Page.navigate had to be moved to inside the navigate event handler, which meant passing some cdp-specific data (the input.id) into the NavigateOpts. This is the only way I found to keep both happy - the sequence of events is closer (but still pretty far) from what Chrome does.
This might not be specific to network notification, but the issue happens all the time testing scenarios that rely on network notification, so it's hard to ignore.
karlseguin
force-pushed
the
http_request_notifications
branch
from
ea3319c to
f59e3cd
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Add 2 internal notifications 1 - http_request_start 2 - http_request_complete
When Network.enable CDP message is received, browser context registers for these 2 events (when Network.disable is called, it unregisters)
On http_request_start, CDP will emit a Network.requestWillBeSent message. This does not include all the fields, but what we have appears to be enough for puppeteer.waitForNetworkIdle.
On http_request_complete, CDP will emit a Network.responseReceived message. This does not include all the fields, bu what we have appears to be enough for puppeteer.waitForNetworkIdle.
We currently don't emit any other new events, including any network-specific lifecycleEvent (i.e. Chrome will emit an networkIdle and networkAlmostIdle).
To support this, the following other things were done:
CDP now has a
notification_arenawhich is re-used between browser contexts. Normally, CDP code runs based on a "cmd" which has its own message_arena, but these notifications happen out-of-band, so we needed a new arena which is valid for handling 1 notification.HTTP Client is notification-aware. The SessionState no longer includes the *http.Client directly. It instead includes an http.RequestFactory which is the combination fo the client + a specific configuration (i.e. *Notification). This ensures that all requests made from that factory have the same settings.
However, despite the above, some requests do not appear to emit CDP events, such as loading a <script src="X">. So the page still deals directly with the *http.Client.
Playwright and Puppeteer (but Playwright in particular) are very sensitive to event ordering. These new events have introduced additional sensitivity. The result sent to Page.navigate had to be moved to inside the navigate event handler, which meant passing some cdp-specific data (the input.id) into the NavigateOpts. This is the only way I found to keep both happy - the sequence of events is closer (but still pretty far) from what Chrome does.