-
Notifications
You must be signed in to change notification settings - Fork 9.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
core(target-manager): only listen to LH sessions #14385
Conversation
Hmm seems to have made the problem worse actually |
Looks like the latest puppeteer was necessary for this |
Seems like this has fixed or at least reduced the likelihood of an However, the Puppeteer upgrade seems to have dramatically increased the likelihood of a I'll keep this open but we should resolve the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR suggests that sessionattached
was fired only on the rootConnection.
And it suggests that with the pptr version bump to 17, it's not reliably emitted there, but instead within child connections.
Is that true?
( As we've done a lot to clean up our target management recently, I'd like to be specific and surgical in making any changes. )
It is reliably emitted, but on In addition to the sessions automatically created by Puppeteer, there are sessions we create when we call This PR changes things so that we only listen to |
FWIW |
It was somewhat flaky but I don't think I ever saw |
from some of the discussion from last week:
|
I have a more concrete explanation now.
Puppeteer automatically creates its own sessions and emits them via We are still creating our own sessions, but they are ignored if a Puppeteer created session on the same target has already been handled:
BTW this return ^ statement isn't the end of the function because it is wrapped in a lighthouse/core/gather/driver/target-manager.js Lines 142 to 145 in 5e6ee16
This change isn't designed to eliminate extra requests. It's to prevent us from using sessions that Puppeteer has already resumed before we call The missing requests can come from missing
Requests that had a |
https://github.com/GoogleChrome/lighthouse/actions/runs/3108583849/jobs/5038074267 Possibly introduced by Puppeteer 18.03 -> 18.05 or something in DT or maybe its just more flakiness. In any case, this PR alone probably shouldn't close #14271 |
This PR upgrades Puppeteer to 18 for two reasons
session.on('sessionattached')
events instead ofconnection.on('sessionattached')
events should closeoopif-requests
flaky on windows #14317 (see below for explanation)lantern-idle-callback-short
is flaky #14271 (see issue for explanation)Upgrading Puppeteer independently of target manager changes increased the likelihood of
oopif-request
failures (#14398), so I bundled these changes together.I was not able to reproduce the
oopif-requests
failure locally. However, I could find unsolicited network responses when I narrowed the focus of my testing to just network manager:I believe that the issues were caused by sessions created by Puppeteer being emitted into our session handler. TBH I didn't figure out the exact details for why this was a problem.However, restricting our session handlers to handle only the sessions that we create seems to have fixed the issue. That is what is being done in this PR. We should run CI a few times as per usual.See #14385 (comment) for explanation.