fix: try to remove the temporary user data directory after the proces… #7761
Conversation
…s has been killed
whimboo
force-pushed
the
kill_temporary_folder
branch
from
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.
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.
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.
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
|
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.