Skip to content
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

misc: handle connection errors even before the timeout #8583

Merged
merged 3 commits into from
Apr 24, 2019
Merged

Conversation

patrickhulce
Copy link
Collaborator

Summary
Should fix travis and give us better error messages when Chrome crashes unexpectedly.

Previously we only handled connection errors after we reached our timeout when we should be handling them as soon as we try to connect in case Chrome crashes unexpectedly.

Related Issues/PRs
#7534

@patrickhulce
Copy link
Collaborator Author

patrickhulce commented Apr 24, 2019

So the errors smoke test has been fixed :)

but the NO_TRACING_STARTED is still very in our face, perhaps #7122 is more of a problem than we realized :(

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so what changed with 74 exactly?

lighthouse-core/gather/connections/cri.js Outdated Show resolved Hide resolved
@patrickhulce
Copy link
Collaborator Author

patrickhulce commented Apr 24, 2019

so what changed with 74 exactly?

Chrome's shutdown procedure is clearly different though I'm not sure what exactly. If I had to guess I would say it's related to network service since that's dramatically changed how Chrome shuts down (usually takes 4-6s now locally).

EDIT: If you were asking what the failure mode was before this, it was that Chrome closes the connection before get a chance to close it ourselves and because we don't handle the error event on the request, node exits prematurely before we can log anything about the error.

@brendankenny brendankenny merged commit 90ae485 into master Apr 24, 2019
@brendankenny brendankenny deleted the fix_master branch April 24, 2019 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants