[🚀 Feature]: Python: better cleanup on `Service.stop()`

[calestyo]

Feature and motivation

Hey.

I'm using selenium with WebKitWebDriver (which feels a bit more lightweight, and is packaged for Debian, unlike e.g. geckodriver) for some automation tasks.

Not sure if this is a bug, but it doesn't just work to do e.g.:

driver = webdriver.WebKitGTK()

(which errors out with Message: Unable to obtain driver for MiniBrowser; For documentation on this error, please visit: https://www.selenium.dev/documentation/webdriver/troubleshooting/errors/driver_location?) but I really have to do e.g.

service = webdriver.WebKitGTKService(executable_path="/usr/bin/WebKitWebDriver", port=8080)
driver = webdriver.WebKitGTK(service=service)

which apparently starts WebKitWebDriver with -p 8080.

What I wanted to make sure is, that if my program ends/dies for whatever reason (including signals, well except SIGKILL[0]) then any of these subprocesses it starts are also killed.

I did something like:

import atexit
import signal

def cleanup():
    # the cleanup

def handler(signalnum, frame):
    sys.exit(128 + signalnum)

for s in (signal.SIGHUP, signal.SIGINT, signal.SIGQUIT, signal.SIGABRT, signal.SIGTERM):
    signal.signal(s, handler)
atexit.register(cleanup)

(the signal handler is needed because atexit would otherwise only work forSIGINT).

With WebKitWebDriver I get a whole bunch of child processes:

    PID    PPID    PGID   TPGID    SESS COMMAND
 283438  228886  283438  283438  228886 /usr/bin/python3 ./x.py
 283439  283438  283439  283438  228886   /usr/bin/WebKitWebDriver --port=8080
 283442  283439  283439  283438  228886     /usr/lib/x86_64-linux-gnu/webkit2gtk
 283466  283442  283439  283438  228886       /usr/lib/x86_64-linux-gnu/webkit2g
 283478  283442  283439  283438  228886       /usr/lib/x86_64-linux-gnu/webkit2g
 283441  283438  283438  283438  228886   [true] <defunct>

Now my natural candidate for # the cleanup would have been to call service.stop(), but that only kills the parent process (PID 283439 above), the various webkit2gtk... remain forever. Ugly.

I've seen Service.send_remote_shutdown_command() but TBH, I don't quite understand what it should do or whether one should use it.

It does at least not stop and processes (and especially not the children).

The main requests of this issue would be:

1. Auto-handle that more properly. I.e. if you start a process, do so in a new process group and kill that (may not work on Windows?).. or something like that?
   
   Perhaps along the way, export the PGID for the user?
2. And/or allow to use an executable_path= of e.g. None and don't start anything, but leave that task to the user (port=) would still be needed of course.

That's what I did now as a workaround, i.e. something like:

process = subprocess.Popen( ("/usr/bin/WebKitWebDriver", "--port=8080"), stdin=subprocess.DEVNULL, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, process_group=0)

which I do before starting with selenium… and my cleanup handler is something like:

            process_group_id = process.pid
            
            os.killpg(process_group_id, signal.SIGTERM)
            process.terminate()
            
            time.sleep(0.5)
            
            os.killpg(process_group_id, signal.SIGKILL)
            process.kill()
            
            process.wait()

(with the process.terminate() and process.kill() being superfluous).

Selenium still tries to start it on it's own…

doesn't even notice that this fails, because the port is already bound to by my manually started driver, which I think should be a bug on it's own and then uses mine.

I can do:

service = webdriver.WebKitGTKService(executable_path="/bin/true", port=8080)

But that's still ugly (this is btw. the zombie true process in he listing above.

Thanks,
Chris.

[0] Might be possible somehow at least on Linux with prctl() and PR_SET_PDEATHSIG, but haven't found out how to use that on a process group (guess it's not directly possible).

Usage example

see above

[github-actions]

@calestyo, thank you for creating this issue. We will troubleshoot it as soon as we can.

---

Info for maintainers

Triage this issue by using labels.

If information is missing, add a helpful comment and then I-issue-template label.

If the issue is a question, add the I-question label.

If the issue is valid but there is no time to troubleshoot it, consider adding the help wanted label.

If the issue requires changes or fixes from an external project (e.g., ChromeDriver, GeckoDriver, MSEdgeDriver, W3C), add the applicable G-* label, and it will provide the correct link and auto-close the issue.

After troubleshooting the issue, please add the R-awaiting answer label.

Thank you!

[cgoldberg]

service.stop() is what stops the driver (that also calls service.send_remote_shutdown_command() internally). This is called during driver.quit(), so I guess your cleanup should call driver.quit() or service.stop(). That shouldn't leave behind zombie child processes. If it does, that's a bug.

If it does leave zombies behind...

Unfortunately, the process_group arg was only added to subprocess.Popen constructor in Python 3.11, and we need to support 3.9+.

https://docs.python.org/3/library/subprocess.html#popen-constructor

The docs are a little unclear on how to set a process group in versions pre-3.11 (using os.setpgrp() and preexec_fn?). I think it can probably be done, but will be more involved than just adding the process_group arg and then calling os.killpg(). Also, process groups are POSIX only, and however it's implemented would have to consider Windows (unclear if the process group stuff is ignored on Windows or raises an error?) and will have to be tested.

@calestyo can you verify driver.quit() or service.stop() leaves zombie child processes behind? If so, are you up for trying to add a fix for this and submitting a Pull Request? If not, I can try when I get some time, or someone else can have a look.

[cgoldberg]

@calestyo

I just did some testing and I don't believe this is an issue. Service.stop() seems to shut down everything correctly and not leave any child processes behind.

Firstly, this code works for me for launching WebKitGTK... It finds the executable on my PATH. I'm not sure why it doesn't for you:

from selenium import webdriver
driver = webdriver.WebKitGTK()

Anyway... using your code, I created a script named sel.py containing this:

import atexit
import signal
import sys
import time

from selenium import webdriver


def cleanup():
    driver.quit()

def handler(signalnum, frame):
    sys.exit(128 + signalnum)

for s in (signal.SIGHUP, signal.SIGINT, signal.SIGQUIT, signal.SIGABRT, signal.SIGTERM):
    signal.signal(s, handler)

atexit.register(cleanup)

service = webdriver.WebKitGTKService(executable_path="/usr/bin/WebKitWebDriver", port=8080)
driver = webdriver.WebKitGTK(service=service)
driver.get('https://example.com')
time.sleep(1000)

It launches the WebKitGTK MiniBrowser, loads a page, and then sleeps. I can run it, then send it a SIGTERM, and it cleans up properly. The python process is terminated, and so are all the WebKitGTK processes.

(venv) cgoldberg617@penguin:~/code$ python sel.py &
[1] 3984
(venv) cgoldberg617@penguin:~/code$ ps | grep -i webkit | grep -v grep
 3988 pts/1    00:00:00 WebKitWebDriver
 4015 pts/1    00:00:00 WebKitNetworkPr
 4031 pts/1    00:00:01 WebKitWebProces
(venv) cgoldberg617@penguin:~/code$ kill -15 3984
(venv) cgoldberg617@penguin:~/code$ ps | grep -i webkit | grep -v grep
[1]+  Exit 143                python3 sel.py

So, AFAICT, everything works fine. You just need to call driver.quit() in your cleanup.

I am going to close this as a non-issue. If this doesn't work for you, I'll re-open it again.

(I'm running selenium 4.29 on Debian Stable with webkit2gtk-driver from the default repos)

[calestyo]

Hey.

service.stop() is what stops the driver (that also calls service.send_remote_shutdown_command() internally). This is called during driver.quit(), so I guess your cleanup should call driver.quit() or service.stop(). That shouldn't leave behind zombie child processes. If it does, that's a bug.

Okay let me see:

1. If in my cleanup() function, I do only service.stop(), then with Ctrl-C, all processes are cleaned up (because I guess the shell simply sends the signal to all processes in the pgroup).
   With a direct SIGIN, SIGTERM, etc. to my python process, the WebKitWebDriver gets cleaned up (I presume by the service.stop(), but not the children of that.

2. If in my cleanup() function, I do only driver.quit(), as you did, then:

   • Ctrl-C gives me the above exception, and again all processes cleaned up.
   • directly sending INT, TERM, etc. to my Python process, doesn't give me an exception, all processes cleaned up

Any idea about the exception?

If it does leave zombies behind...

I guess the zombie true process was merely because the process terminated but since selenium was still using my manually started WebKitWebDriver on 8080, it didn't bother to wait for the process (true) it started itself and thus that wasn't reaped.

Unfortunately, the process_group arg was only added to subprocess.Popen constructor in Python 3.11, and we need to support 3.9+.

https://docs.python.org/3/library/subprocess.html#popen-constructor

The docs are a little unclear on how to set a process group in versions pre-3.11 (using os.setpgrp() and preexec_fn?).

Yes I think pre-3.11 it would be done like that, using 0, 0 as args.
I was also thinking about using setsid() (which also does a new pgroup, but in addition a new session, but I felt a new pgroup would be enough).

I think it can probably be done, but will be more involved than just adding the process_group arg and then calling os.killpg(). Also, process groups are POSIX only.

Indeed, as I've said. Not sure how easy it would be for you to simply handle things differently between Windows/POSIX.
For Windows I've seen subprocess.CREATE_NEW_PROCESS_GROUP, but not sure whether that can be used in a similar way.

@calestyo can you verify driver.quit() or service.stop() leaves zombie child processes behind? If so, are you up for trying to add a fix for this and submitting a Pull Request? If not, I can try when I get some time, or someone else can have a look.

I really think the zombie is just because of my above abuse of your interface, i.e. using executable_path="/bin/true" to prevent selenium to start WebKitWebDriver by itself.

That's why I've asked in the original report the point (2), i.e. whether you could support e.g. executable_path=None to not start anything on your own.

I just did some testing and I don't believe this is an issue. Service.stop() seems to shut down everything correctly and not leave any child processes behind.

Well thanks for the hint with .quit().

I haven't found that in the documentation... in fact, https://selenium-python.readthedocs.io/api.html seems to completely lack the API for webkitgtk (which I found now at https://www.selenium.dev/selenium/docs/api/py/webdriver_webkitgtk/selenium.webdriver.webkitgtk.webdriver.html).

That's why I thought .stop() would be the way to go. ^^

https://selenium-python.readthedocs.io/api.html#selenium.webdriver.common.service.Service.start could perhaps be appended to notice that this stops only the driver process itself, not and children?

Thanks and sorry for the noise.

[calestyo]

Firstly, this code works for me for launching WebKitGTK... It finds the executable on my PATH. I'm not sure why it doesn't for you:

Strange. I have a standard PATH and WebKitWebDriver is in it. May I ask what your:

/usr/lib/python3/dist-packages/selenium/webdriver$ grep -R DEFAULT_EXECUTABLE_PATH
webkitgtk/service.py:DEFAULT_EXECUTABLE_PATH = "WebKitWebDriver"

looks like?

But still subprocess should search in the PATH.

[cgoldberg]

2. If in my cleanup() function, I do only driver.quit(), as you did, then:

   • Ctrl-C gives me the above exception, and again all processes cleaned up.
   • directly sending INT, TERM, etc. to my Python process, doesn't give me an exception, all processes cleaned up

Any idea about the exception?

I'm not really sure about the exception. I thought Ctrl-C would be the same as sending the signal, but I guess not? Anyway, I'm glad calling driver.quit() cleans everything up. That's the proper way to quit everything.

I really think the zombie is just because of my above abuse of your interface, i.e. using executable_path="/bin/true" to prevent selenium to start WebKitWebDriver by itself. That's why I've asked in the original report the point (2), i.e. whether you could support e.g. executable_path=None to not start anything on your own.

I never considered that case, but we could probably support not starting anything if executable_path=None is passed. Can you submit a PR for that?

I haven't found that in the documentation... in fact, https://selenium-python.readthedocs.io/api.html seems to completely lack the API for webkitgtk

That's not official documentation, and I wish the author would just remove it. It's very outdated. Several years ago I got him to add a disclaimer that it's not official documentation... but it still exists and causes confusion.

https://www.selenium.dev/selenium/docs/api/py/webdriver_webkitgtk/selenium.webdriver.webkitgtk.webdriver.html

That one is updated every release and is auto-generated from docstrings in the code.

could perhaps be appended to notice that this stops only the driver process itself, not and children?

I suppose we could add a note to the docstring for stop() that it doesn't stop children. However, I'm still not exactly sure why it doesn't. It's probably not intended to be called by end users, and should probably be renamed to indicate that. The whole stop/start process across all the drivers is really confusing and hard to follow. My head hurts every time I try to figure out how it all works.

[cgoldberg]

@calestyo

/usr/lib/python3/dist-packages/selenium/webdriver$ grep -R DEFAULT_EXECUTABLE_PATH
webkitgtk/service.py:DEFAULT_EXECUTABLE_PATH = "WebKitWebDriver"

aha! I wasn't aware of this: #15046

That explains why it's not finding it in your PATH. You must be on an older version of selenium. That fix was in selenium version 4.28 (current version is 4.29).

[github-actions]

This issue has been automatically locked since there has not been any recent activity since it was closed. Please open a new issue for related bugs.