Cross-platform browser process killer #110
Conversation
inancgumus
force-pushed
the
feat/gc-browser-processes
branch
from
b53b791 to
cb817d0
Compare
* Saves the process ID to the context when launching a new browser. * k6Throw helper function kills the lingering processes before throwing a k6 error.
inancgumus
force-pushed
the
feat/gc-browser-processes
branch
from
cb817d0 to
ef87493
Compare
There was a problem hiding this comment.
Just some nitpicks and a question.
I was in the middle of reviewing it when you removed the psutil dependency :), but it's better if we can do this with the stdlib.
Some debug logging would be nice to know what's happening, and tests of course.
Also, why aren't we replacing the other direct uses of k6common.Throw()?
;)
PR description :) "We need to update k6common.Throw to our k6Throw for this to work stable. ... Automated tests are missing for now. I'm planning to do these in a separate PR." |
There was a problem hiding this comment.
The code LGTM, but I wasn't able to come up with a manual test for this scenario.
On main, stopping the test run with Ctrl+C or calling k6.fail() in the script properly kills the process and leaves no zombies behind. Panic issues also seem to get rid of the process. 😕 So I'm not sure about the exact scenario that reproduces this, but automated tests (E2E only I suppose) would help.
Since I don't see the code doing any harm, I'll merge this as is.
This PR adds a cross-platform browser process killer.
k6Throwkills any lingering browser processes in a cross-platform way.We need to update
k6common.Throwto ourk6Throwfor this to work stable. And I manually tested this with 100 browsers on my Mac. It cleaned up all the lingering processes. Automated tests are missing for now. I'm planning to do these in a separate PR.As I said in the PR this is a temporary fix:
#93 (comment)
Closes: #93