revise taskkill procedure on windows #267
Conversation
| } | ||
| } | ||
| } catch (err) { | ||
| const message = `Chrome could not be killed ${err.message}`; | ||
| log.warn('ChromeLauncher', message); | ||
| reject(new Error(message)); |
There was a problem hiding this comment.
The smoke tests still pass even with this rejection right?
There was a problem hiding this comment.
no they don't.
removing this rejection is deliberate actually. as mentioned in #266, pptr and pw deliberately do not reject in this case.
and i've been convinced that this is reasonable. there appears to be a race condition and taskkill is attempting to kill processes that are already gone. what's interesting is that when this happens. the processes that fail to be killed (cuz they're not found) are all child proc of the primary chrome proc. the chrome proc is always successfully killed.
take a peek at the SO item linked in microsoft/playwright#7500 .... PW used this MEMUSAGE hack for a while but then determined it wasnt needed. so i'm following their latest pattern.
i think it's still important we have some visibility on when taskkill returns an error, but it's not a fatal situation.
There was a problem hiding this comment.
They removed it because
Since then, we no longer inherit stdout/stderr, so the error message
should not appear anymore.
Which we also do not do so lgtm.
src/chrome-launcher.ts
Outdated
| if (this.chrome) { | ||
| log.log('ChromeLauncher', `Chrome already running with pid ${this.chrome.pid}.`); | ||
| return this.chrome.pid; | ||
| if (this.chromeProc) { |
There was a problem hiding this comment.
nit: chromeProcess
I'm reading "chromeProc" as "chromeProcedure" (because that is a weird/old way to shorten that word) so reading was confusing
mostly fixes #266
adopted much of playwright's logic (detailed here)
shell:trueswapped in forstdio:pipekill()matching pptr & pw behavior.also:
this.chrometothis.chromeProc(sorry for the noise, but we have anotherchromein the public API that's a different thing)(currently on top of #265 ... lighthouse-smoke...adopt-pw-kill )