-
Notifications
You must be signed in to change notification settings - Fork 9k
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
fix: try to remove the temporary user data directory after the proces… #7761
Conversation
…s has been killed
3c6662b
to
3c6dc5d
Compare
@OrKoN and @jschfflr, even with no actual improvement for Firefox because the process cannot be correctly killed (see #7668 (comment)), that change would make sense anyway and we can benefit at a later time. Not sure about Chrome and why it's not failing there. |
@@ -199,6 +192,14 @@ export class BrowserRunner { | |||
); | |||
} | |||
} | |||
|
|||
// Attempt to remove temporary profile directory to avoid littering. |
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.
What if this.proc.kill('SIGKILL') throws but the process is still killed? Can that happen in practice? That might be the reason for removing the temp dir upfront?
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.
Good question. So I've taken a look at the subprocess.kill() documentation, and I cannot see that an error gets thrown at all. So my question would be do we really need the try/catch and will we ever re-throw the error (that is the part that I totally issed - thanks for noting!).
Shouldn't a check for the return value be performed instead? We could then throw an error at the end of the method, and could still have the user data directory deletion before. If it is false
we could still try to remove the user data directory even if not fully successful.
Maybe the error
and close
events could be used here to better handle the kill status by making the BrowserRunner.kill()
method async.
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.
Yeah, that's strange. I could not find why it was like this in the first place. It does not look like it can throw https://github.com/nodejs/node/blob/master/lib/child_process.js#L419
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.
since .kill() does not throw, LGTM. We could also clean up the try catch.
Thank you for merging! |
Seen while investigating issues on Windows via #7668. We are trying to remove the temporary user data directory before the process is killed. That means that the process has still hold on some of the files, and removing the folder will definitely fail.
A try to remove the folder should happen afterward.