Hi @mcepl and thank you for your contribution!

Notice that the CI is failing right now with this error:

Traceback (most recent call last):
  File "/home/travis/build/python/cpython/Lib/test/test_threading.py", line 1194, in test_interrupt_main_error
    handler = signal.getsignal(signal.SIGINT)
NameError: name 'signal' is not defined

This is because you are using the signal module in the tests (test_interrupt_main_error) but you did not import it. Notice that to avoid waiting for the CI to fail in the PR you can run your tests locally. For example:

./configure --with-pydebug && make && make test

The tests keep failing:

======================================================================
ERROR: test_interrupt_main_error (test.test_threading.InterruptMainTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/python/cpython/Lib/test/test_threading.py", line 1199, in test_interrupt_main_error
    _thread.interrupt_main()
TypeError: 'int' object is not callable
----------------------------------------------------------------------
Ran 161 tests in 12.431s

Please, remember to run the tests locally to check that the patch works :)

Please, remember to run the tests locally to check that the patch works :)

This was marked as [WIP] as recording of the patch for bug 23395 by @vstinner . I thought it is better to have it here even broken, so somebody who knows what's going on can take a look, than this patch be forever lost in bowls of the issue tracking system.

I personally don't understand why _thread.interrupt_main() which seems to me like a nice function, suddenly changes to int, so I cannot help with fixing it.

The tests are not passing because you did not apply the patch completely. You missed

diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c
index 69e27be9cc..d3ed1b9acf 100644
--- a/Modules/_threadmodule.c
+++ b/Modules/_threadmodule.c
@@ -1103,7 +1103,9 @@ thread to exit silently unless the exception is caught.");
 static PyObject *
 thread_PyThread_interrupt_main(PyObject * self, PyObject *Py_UNUSED(ignored))
 {
-    PyErr_SetInterrupt();
+    if (PyErr_SetInterruptWithErr() < 0) {
+    return NULL;
+    }
     Py_RETURN_NONE;
 }

you need to add the function to the public API:

diff --git a/Include/pyerrors.h b/Include/pyerrors.h
index 416d750d9b..5011f601d1 100644
--- a/Include/pyerrors.h
+++ b/Include/pyerrors.h
@@ -356,6 +356,7 @@ PyAPI_FUNC(PyObject *) _PyErr_TrySetFromCause(
 /* In signalmodule.c */
 PyAPI_FUNC(int) PyErr_CheckSignals(void);
 PyAPI_FUNC(void) PyErr_SetInterrupt(void);
+PyAPI_FUNC(int) PyErr_SetInterruptWithErr(void);

@vstinner What's missing from this PR to be merged?

@pitrou Could I ask for a review, here (and/or merge)?

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

I have a problem with this PR. It doesn't fix PyErr_SetInterrupt at all, it requires the caller to switch to another API. I think it would make sense to fix PyErr_SetInterrupt so that it does nothing if SIGINT isn't handled by Python.

@pitrou Would you be OK with this pull request, if instead of creating new function, we would just change PyErr_SetInterrupt to the proposed reading of PyErr_SetInterruptWithErr as it stands right now and not create new function at all?

we would just change PyErr_SetInterrupt to the proposed reading of PyErr_SetInterruptWithErr as it stands right now and not create new function at all?

Do you mean raise an error or not? I don't think it's worthwhile to raise an error, TBH. These are low-level API functions that are generally not a good idea anyway. @vstinner may disagree.

Do you mean raise an error or not? I don't think it's worthwhile to raise an error, TBH. These are low-level API functions that are generally not a good idea anyway. @vstinner may disagree.

I came to this pull request from https://bugzilla.opensuse.org/1094814 , so there is some real reason for it. What do you think should be the ideal solution for that bug? @pitrou @vstinner

This PR was in my TODO list (of PRs that I should review), but sadly I failed to find the bandwidth to review it. Don't hesitate to retry to ping me in two weeks if nobody came up with a review in the meanwhile ;-)

@vstinner ping ;)

We encounter similar problem but in signal.signal function instead of interrupt_main. This pull request does not seem to solve it, but I was wondering if the source of the problem is the same?

Following code can reproduce the problem:

import signal
import os
import sys
import time

import multiprocessing

def fail():
    def handler(*args):
        pass

    while True:
        signal.signal(signal.SIGUSR1, signal.SIG_IGN)
        signal.signal(signal.SIGUSR1, handler)

proc = multiprocessing.Process(target=fail)
proc.start()
time.sleep(1)
pid = proc.pid

i = 0
try:
    while proc.is_alive():
        os.kill(pid, signal.SIGUSR1)
        time.sleep(0.001)
finally:
    proc.join()

Tested on this branch, it fails with:

Process Process-1:
Traceback (most recent call last):
  File "/home/plocharz/projects/cpython/Lib/multiprocessing/process.py", line 302, in _bootstrap
    self.run()
  File "/home/plocharz/projects/cpython/Lib/multiprocessing/process.py", line 99, in run
    self._target(*self._args, **self._kwargs)
  File "keep-killing.py", line 13, in fail
    signal.signal(signal.SIGUSR1, signal.SIG_IGN)
  File "/home/plocharz/projects/cpython/Lib/signal.py", line 47, in signal
    handler = _signal.signal(_enum_to_int(signalnum), _enum_to_int(handler))
TypeError: 'int' object is not callable

The feature "It may be called without holding the interpreter lock" of PyErr_SetInterrupt is important. It should be kept in the documentation. I think it's also a reason why it shouldn't be deprecated.

I find the wording "The :data:signal.SIGINT signal must be handled by Python" unclear. What does "must be handled by Python" mean? I suggest something like "a signal handler for the :data:signal.SIGINT signal must have been installed by the signal function".

@serhiy-storchaka @pablogsal Could I ask for merge here, please? What remains to be fixed?

I have a problem with this PR. It doesn't fix PyErr_SetInterrupt at all, it requires the caller to switch to another API. I think it would make sense to fix PyErr_SetInterrupt so that it does nothing if SIGINT isn't handled by Python.

@pitrou Is this what you meant? Or should I go even further and make those RuntimeErrors into mere warnings?

I don't know if it's ok to change the return type of PyErr_SetInterrupt, because of the stable ABI. @vstinner @serhiy-storchaka What do you think?

(otherwise I'm ok with the PR on the principle)

I don't know if it's ok to change the return type of PyErr_SetInterrupt, because of the stable ABI. @vstinner @serhiy-storchaka What do you think?

(otherwise I'm ok with the PR on the principle)

Right, it is possible not to return anything and just to rely on exceptions to report exceptional state, but I thought that returning errorlevel is more standard C way of doing things. You decide.

My original suggestion was to do nothing if there is no SIGINT signal handler. Don't raise an exception, don't print a warning.

Edit: PyErr_SetInterrupt can be called without the GIL (see docs), so we can't raise an exception or emit a warning.

My original suggestion was to do nothing if there is no SIGINT signal handler. Don't raise an exception, don't print a warning.

Hmm, that would do, but it seems unkind not to tell users something is wrong.

Edit: PyErr_SetInterrupt can be called without the GIL (see docs), so we can't raise an exception or emit a warning.

So, in the end we have to return errorlevel to signal anything?

BTW, @pitrou, could you do something about that pending review? I think it is completely obsolete now (I am sorry for that rebase) and I don't think I have a way how to get rid off it.

So, in the end we have to return errorlevel to signal anything?

If we want to return an error, yes, we need a new API. But the existing API should still be fixed.

So, in the end we have to return errorlevel to signal anything?

If we want to return an error, yes, we need a new API. But the existing API should still be fixed.

OK, so this one? Yes, we may discuss some alternative API later, but I would love to get this into 3.8 so let's keep it as simple as possible.

On the principle, yes. The PR needs polishing, I'll do it.

Thanks @mcepl for the PR, and @pitrou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

On the principle, yes. The PR needs polishing, I'll do it.

You are right, I forgot try ... finally. That's handy.

Thanks @mcepl for the PR, and @pitrou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

GH-13541 is a backport of this pull request to the 3.7 branch.