bpo-38630: subprocess: enhance send_signal() on Unix

[vstinner]

On Unix, subprocess.Popen.send_signal() now polls the process status.
Polling reduces the risk of sending a signal to the wrong process if
the process completed, but its identifier has been reassigned
(recycled) to a new different process.

https://bugs.python.org/issue38630

[vstinner]

For the test, I tried to call os.kill(pid, 0) in a loop to wait until the process completes without calling waitpid(). os.kill(pid, 0) doesn't fail even if the process already completes. It only starts failing when waitpid() is called.

[vstinner]

@gpshead: Would you mind to review this change? The additional poll() call should be cheap in term of performance and it makes the code "more correct" (reduce the risk of race condition).

[gpshead]

feel free to backport this to 3.8 if you want.

[vstinner]

Note for myself: I would like to elaborate a few comments before merging the change. I need to clarify in which cases the polling is required.

[vstinner]

@gpshead: I elaborated send_signal() comment. Would you mind to review my updated PR? I also added a comment explaining why my change does not fully fix the race condition, it only reduces the risk.

I would like to backport this bugfix to Python 3.7 and 3.8 branch.

---

By the way, what do you have think of modifying send_signal(), terminate() and kill() return value? Return True if a signal has been set, or False if the method did nothing.

The return value must not be modified in micro releases (3.7 and 3.8 branches). I'm only talking about the master branch (future Python 3.9).

I will propose a separated PR once this one is merged.

[vstinner]

cc @giampaolo

[gpshead]

seems fine for 3.7 and 3.8 as well. i waffled on trying to enhance the wording in the docs to mention the possible race condition, but those are all unusual circumstances not worth laying out in the docs for send_signal.

[vstinner]

i waffled on trying to enhance the wording in the docs to mention the possible race condition, but those are all unusual circumstances not worth laying out in the docs for send_signal.

I agree :-)

[vstinner]

@giampaolo, @njsmith: I understand that there are other approaches to fix https://bugs.python.org/issue38630 but are you ok with this change which is simple enough and reduce the risk?

[njsmith]

It's not clear to me that this actually does reduce the risk. Merging won't do any harm; it's a no-op at worst. But the comment seems misleading, and you should make sure the bug isn't marked as closed.

[giampaolo]

I don't have a strong opinion about this change. It seems it does not do any harm so it can probably be committed, but at the same time it does not solve the original problem either. For that (avoiding to kill() a reused process PID) the right solution IMO is using the process creation time. It appears to be more portable than using the pidfd API(s) as it's Linux only (?).

[njsmith]

I don't see any way process creation time can fix the race condition (kill doesn't take a process creation time!), but that discussion should probably go in the bpo issue. That issue also has another suggestion that I think does actually solve the race condition.

[csabella]

Should this be flagged for additional discussion or is it ready to be merged? Thanks!