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

Eliminate errors from calling ChromeLauncher.kill() twice #1131

Merged
merged 10 commits into from
Dec 14, 2016

Conversation

Janpot
Copy link
Contributor

@Janpot Janpot commented Dec 9, 2016

The error happened outside the Promise chain and is uncatchable.

The error happened outside the Promise chain and is uncatchable.
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@Janpot
Copy link
Contributor Author

Janpot commented Dec 9, 2016

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@wardpeet
Copy link
Collaborator

wardpeet commented Dec 9, 2016

can you tell us when this exactly happens?

@Janpot
Copy link
Contributor Author

Janpot commented Dec 9, 2016

reproducible when doing

launcher.kill();
launcher.kill();

Nobody would ever do exactly this but in some flows this might happen. In my particular case I had set up a SIGINT handler that kills the launcher. Then set up development with nodemon, which sends a SIGINT when it's restarting. But in the case of manually interrupting nodemon the signal gets sent twice.

Regardless of this corner case though, the existing code has a possibility of throwing an error that can't be caught by any consuming code. closeSync can throw.

In the kill() method there is a comment

// fail silently as we did not start chrome

The fact that this comment exists means to me that trying to kill a non-running launcher is defined behavior. And the behavior is 'fail silently', which I implemented for the case where you try to kill an already killed launcher.

There's also other ways of fixing this:

  • set this.outFile = null; and this.errFile = null; after closing them.
  • set this.chrome = null; at the end of the kill method.

@wardpeet
Copy link
Collaborator

wardpeet commented Dec 9, 2016

I would go for

set this.outFile = null; and this.errFile = null; after closing them.
set this.chrome = null; at the end of the kill method.

Than we can keep everything sync :)

@brendankenny
Copy link
Member

There's also other ways of fixing this:

  • set this.outFile = null; and this.errFile = null; after closing them.
  • set this.chrome = null; at the end of the kill method.

even if you went the way of the current PR these should really be nulled after closing anyways, so it would be great to go that way

@Janpot
Copy link
Contributor Author

Janpot commented Dec 10, 2016

@wardpeet is there any specific reason you prefer the sync methods over the async ones? The main reason why I changed this code is because I want to be able to handle the errors that might possibly be thrown by closeSync. Using promises to do that there just felt more natural to me. Would you rather want me to use a try {} catch {} and the closeSync method instead?

@wardpeet
Copy link
Collaborator

wardpeet commented Dec 10, 2016

It's only my personal experience if you don't use async await, promises make your code more complex than using plain sync when it's not necessary.

* add nulling of properties
* make synchronous again
@Janpot
Copy link
Contributor Author

Janpot commented Dec 10, 2016

I disagree, but I don't think it's worth starting an argument over. I updated it, can you take another look?

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.

it could be good to add a unit test that we avoid this error, but we can do that in a future PR if you'd rather move forward with the scope of this one.

if (this.outFile) {
fs.closeSync(this.outFile);
}
try {
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather avoid the try/catch since it'll silently eat errors, but is it even necessary? This is the only place that these files are closed, so the conditionals should guard against attempting to delete with already-closed file descriptor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wrapped the whole function in the promise, instead of only rimraf

@Janpot
Copy link
Contributor Author

Janpot commented Dec 13, 2016

@brendankenny Added a test

@brendankenny brendankenny changed the title Calling .kill() twice results in EBADF error in closeSync Eliminate errors from calling ChromeLauncher.kill() twice Dec 13, 2016
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.

a few last style nits

rimraf(this.TMP_PROFILE_DIR, function() {
resolve();
});
rimraf(this.TMP_PROFILE_DIR, () => resolve());
Copy link
Member

Choose a reason for hiding this comment

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

nit: this can be just rimraf(this.TMP_PROFILE_DIR, () => resolve());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

did you mean rimraf(this.TMP_PROFILE_DIR, resolve);? I chose the former because the return type is Promise<undefined> and the callback to rimraf could return with an error.

Copy link
Member

Choose a reason for hiding this comment

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

ah, good point, SGTM as is


const ChromeLauncher = require('../../chrome-launcher.js').ChromeLauncher;

/* global describe, it */
Copy link
Member

Choose a reason for hiding this comment

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

use /* eslint-env mocha */ instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I usually use but /* global describe, it */ is all over the code base.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, sorry, that's legacy stuff that's slowly getting changed over as those files are touched :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can replace it everywhere if you want, or would that clutter this PR too much?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, I generally prefer keeping PRs contained and we can have a separate PR for that (and/or change as we go since it's functionally equivalent)

const chromeInstance = new ChromeLauncher();
chromeInstance.prepare();
chromeInstance.run();
return chromeInstance.waitUntilReady()
Copy link
Member

Choose a reason for hiding this comment

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

I believe you can do just

const chromeInstance = new ChromeLauncher();
return chromeInstance.run()
  .then(() => {
    ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, it was like that in manual-chrome-launcher. Can change there as well?

Copy link
Member

Choose a reason for hiding this comment

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

sounds good

Copy link
Member

Choose a reason for hiding this comment

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

er, I think it's fine as is. I'd rather avoid touching it so we can land this change

Copy link
Member

Choose a reason for hiding this comment

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

chromeInstance.run() also waits on waitUntilReady() internally, so it isn't necessary to call chromeInstance.waitUntilReady() explicitly

.then(() => {
return Promise.all([
chromeInstance.kill(),
chromeInstance.kill()
Copy link
Member

Choose a reason for hiding this comment

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

👍

@Janpot
Copy link
Contributor Author

Janpot commented Dec 13, 2016

feel free to cancel https://travis-ci.org/GoogleChrome/lighthouse/builds/183750931 to clear up queue

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.

LGTM

Thanks!

@brendankenny brendankenny merged commit 215215e into GoogleChrome:master Dec 14, 2016
@Janpot Janpot deleted the killerror branch December 14, 2016 07:27
andrewrota pushed a commit to andrewrota/lighthouse that referenced this pull request Jan 13, 2017
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.

5 participants