[issue38630] subprocess.Popen.send_signal() should poll the process

Jack O'Connor added the comment: Filed separately as https://bugs.python.org/issue42558. -- ___ Python tracker ___ ___

[issue38630] subprocess.Popen.send_signal() should poll the process

STINNER Victor added the comment: On Linux, a pidfd file descriptor can be created with os.pidfd_open() (added to Python 3.9). It would avoid even more race conditions. The best would be to request a pidfd file descriptor directly when we spawn the process which is possible on recent Linux

[issue38630] subprocess.Popen.send_signal() should poll the process

Jack O'Connor added the comment: This change caused a crash in the Duct library in Python 3.9. Duct uses the waitid(NOWAIT) strategy that Nathaniel Smith has described in this thread. With this change, the child process is reaped by kill() in a spot we didn't expect, and a subsequent call

[issue38630] subprocess.Popen.send_signal() should poll the process

STINNER Victor added the comment: While there was no strong agreement on my change, Nathaniel and Giampaolo said that it should not harm to merge it :-) So I merged my change. It has been said multiple times: my change doesn't fully close the race condition. It only reduces the risk that it

[issue38630] subprocess.Popen.send_signal() should poll the process

STINNER Victor added the comment: New changeset e85a305503bf83c5a8ffb3a988dfe7b67461cbee by Victor Stinner in branch 'master': bpo-38630: Fix subprocess.Popen.send_signal() race condition (GH-16984) https://github.com/python/cpython/commit/e85a305503bf83c5a8ffb3a988dfe7b67461cbee

[issue38630] subprocess.Popen.send_signal() should poll the process

Nathaniel Smith added the comment: > revalidate pid licenses It means autocorrect mangled the text... I don't remember what word that's supposed to be, but basically I meant "revalidate the pid hasn't been reaped" > Wouldn't it be sufficient to add > > if self.returncode is not

[issue38630] subprocess.Popen.send_signal() should poll the process

Роман Донченко added the comment: > I'm pretty sure you mean WNOWAIT, right? Right. > revalidate pid licenses What does this mean? > I think we can patch up this last race condition by adding yet one more lock Wouldn't it be sufficient to add if self.returncode is not None: return

[issue38630] subprocess.Popen.send_signal() should poll the process

Nathaniel Smith added the comment: > Thread B thinks the process is still running, so it calls waitid+WNOHANG on > a stale PID, with unpredictable results. I'm pretty sure you mean WNOWAIT, right? The actual reaping step (which might use WNOHANG) is already protected by a lock, but there's

[issue38630] subprocess.Popen.send_signal() should poll the process

Роман Донченко added the comment: > What you do is split 'wait' into two parts: first it waits for me process to > become reapable without actually reaping it. On Linux you can do this with > waitid+WNOWAIT. On kqueue platforms you can do it with kqueue. > Then, you take a lock, and use it

[issue38630] subprocess.Popen.send_signal() should poll the process

STINNER Victor added the comment: > To further elaborate on the creation time solution, the idea in pseudo-code > is the following: (...) > Technically there is still a race condition between _execute_child() and > get_create_time(), but: (...) Even if the approach seems to be different, PR

[issue38630] subprocess.Popen.send_signal() should poll the process

Giampaolo Rodola' added the comment: To further elaborate on the creation time solution, the idea in pseudo-code is the following: class Popen: def __init__(self, ...): self._execute_child(...) try: self._ctime = get_create_time(self.pid) except

[issue38630] subprocess.Popen.send_signal() should poll the process

Nathaniel Smith added the comment: Hmm, you know, on further thought, it is actually possible to close this race for real, without pidfd. What you do is split 'wait' into two parts: first it waits for me process to become reapable without actually reaping it. On Linux you can do this with

[issue38630] subprocess.Popen.send_signal() should poll the process

Nathaniel Smith added the comment: You can't solve a time-of-check-to-time-of-use race by adding another check. I guess your patch might narrow the race window slightly, but it was a tiny window before and it's a tiny window after, so I don't really get it. The test doesn't demonstrate a

[issue38630] subprocess.Popen.send_signal() should poll the process

STINNER Victor added the comment: > But then deeper in the thread it sounded like you concluded that this didn't > actually help anything, which is what I would expect :-). The PR contains a test which demonstrate one race condition. It fails without the fix. --

[issue38630] subprocess.Popen.send_signal() should poll the process

STINNER Victor added the comment: > (Except maybe eventually switch to pidfd or similar, but Victor says he wants > to use a different issue for that.) pidfd is not available on all platforms (ex: Windows). -- ___ Python tracker

[issue38630] subprocess.Popen.send_signal() should poll the process

Nathaniel Smith added the comment: But then deeper in the thread it sounded like you concluded that this didn't actually help anything, which is what I would expect :-). -- ___ Python tracker

[issue38630] subprocess.Popen.send_signal() should poll the process

STINNER Victor added the comment: > It sounds like there's actually nothing to do here? (Except maybe eventually > switch to pidfd or similar, but Victor says he wants to use a different issue > for that.) Can this be closed? I opened this issue to propose PR 16984. Did you notice the PR?

[issue38630] subprocess.Popen.send_signal() should poll the process

Nathaniel Smith added the comment: It sounds like there's actually nothing to do here? (Except maybe eventually switch to pidfd or similar, but Victor says he wants to use a different issue for that.) Can this be closed? -- nosy: +njs ___ Python

[issue38630] subprocess.Popen.send_signal() should poll the process

Giampaolo Rodola' added the comment: > Did someone propose a pull request to fix this issue by ignoring > ProcessLookupError? I misread your PR, sorry. I thought that was the effect. -- ___ Python tracker

[issue38630] subprocess.Popen.send_signal() should poll the process

STINNER Victor added the comment: > I would prefer to only use platform specific code when the platform provides > something like pidfd: I understand that Linux, FreeBSD, OpenBSD, NetBSD, and > Illumos are concerned. I mean reuse an exisiting solution, rather than > writing our own

[issue38630] subprocess.Popen.send_signal() should poll the process

STINNER Victor added the comment: > -1 about the PR solution to suppress ProcessLookupError in case the process > is gone. Did someone propose a pull request to fix this issue by ignoring ProcessLookupError? > In psutil I solved the “pid reused problem” by using process creation time to >

[issue38630] subprocess.Popen.send_signal() should poll the process

Giampaolo Rodola' added the comment: -1 about the PR solution to suppress ProcessLookupError in case the process is gone. In psutil I solved the “pid reused problem” by using process creation time to identify a process uniquely (on start). A decorator can be used to protect the sensibile

[issue38630] subprocess.Popen.send_signal() should poll the process

STINNER Victor added the comment: > subprocess.Popen.send_signal() doesn't check if the process exited since the > poll() method has been called for the last time. If the process exit just > before os.kill() is called, the signal can be sent to the wrong process if > the process identified

[issue38630] subprocess.Popen.send_signal() should poll the process

STINNER Victor added the comment: I noticed this issue by reading subprocess code while working on bpo-38207 ("subprocess: Popen.kill() + Popen.communicate() is blocking until all processes using the pipe close the pipe") and bpo-38502 ("regrtest: use process groups"). --

[issue38630] subprocess.Popen.send_signal() should poll the process

Change by STINNER Victor : -- keywords: +patch pull_requests: +16510 stage: -> patch review pull_request: https://github.com/python/cpython/pull/16984 ___ Python tracker ___

[issue38630] subprocess.Popen.send_signal() should poll the process

New submission from STINNER Victor : subprocess.Popen.send_signal() doesn't check if the process exited since the poll() method has been called for the last time. If the process exit just before os.kill() is called, the signal can be sent to the wrong process if the process identified is