Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with this file, but it looks like these Py_XINCREF calls should be made after the calls to PyModule_AddObject, correct?

If PyModule_AddObject fails, the additional reference isn't stolen... so it isn't needed.

I'm not familiar with this file, but it looks like these Py_XINCREF calls should be made after the calls to PyModule_AddObject, correct?

If PyModule_AddObject fails, the additional reference isn't stolen... so it isn't needed.

Search PyModule_AddObject in CPython code.
It seems always Py_INCREF first, then call PyModule_AddObject.

If Py_INCREF after calls, maybe the object is already deallocated inside PyModule_AddObject function?
https://github.com/python/cpython/blob/3.8/Python/modsupport.c#L654

No need needs backport to 3.7 label, only 3.8 and 3.9 branches are affected.

This is a regression caused by commit 9541bd3 (22 Apr 2019)

Ah, yes. I at least think that we should Py_XDECREF in the failure condition (before goto finally;) though, right?

Search in CPython code, some sites don't check PyModule_AddObject's return value, some sites don't do Py_DECREF if PyModule_AddObject fails. Looks less normative.

I did all in the last commit.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I have a few minor comments.

@nanjekyejoannah: Would you mind to also review this change? I would prefer to have a second reviewer.

@vstinner It looks like the idiom of calling PyModule_AddObject without Py_DECREF'ing on the error condition (or even checking for it at all) has spread quite a bit since https://bugs.python.org/issue26868 was first reported. I'll prepare a PR to fix the other call sites!

I don't want other contributors to think that this is the correct way to use it.

If you don't mind, I want to reuse PyDict_SetItemString().
It keeps the code neat, and saves a pair of Py_INCREF()/Py_DECREF().

compare

    DefaultHandler = PyLong_FromVoidPtr((void *)SIG_DFL);
    if (!DefaultHandler ||
        PyDict_SetItemString(d, "SIG_DFL", DefaultHandler) < 0) {
        goto finally;
    }

to

    DefaultHandler = PyLong_FromVoidPtr((void *)SIG_DFL);
    if (!DefaultHandler) {
        goto finally;
    }
    Py_INCREF(DefaultHandler);
    if (PyModule_AddObject(m, "SIG_DFL", DefaultHandler)) {
        Py_DECREF(DefaultHandler);
        goto finally;
    }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't mind, I want to reuse PyDict_SetItemString().
It keeps the code neat, and saves a pair of Py_INCREF()/Py_DECREF().

compare

    DefaultHandler = PyLong_FromVoidPtr((void *)SIG_DFL);
    if (!DefaultHandler ||
        PyDict_SetItemString(d, "SIG_DFL", DefaultHandler) < 0) {
        goto finally;
    }

to

    DefaultHandler = PyLong_FromVoidPtr((void *)SIG_DFL);
    if (!DefaultHandler) {
        goto finally;
    }
    Py_INCREF(DefaultHandler);
    if (PyModule_AddObject(m, "SIG_DFL", DefaultHandler)) {
        Py_DECREF(DefaultHandler);
        goto finally;
    }

Follow the discussion here : https://bugs.python.org/issue24011 on replacing PyDict_SetItemString() calls with PyModule_AddIntMacro() . By the time I made this change, all the PyModule_AddIntMacro() changes had been made save for this in this commit 6782b14

Moving this to a comment : Follow the discussion here : https://bugs.python.org/issue24011 on replacing PyDict_SetItemString() calls with PyModule_AddIntMacro() . By the time I made this change, all the PyModule_AddIntMacro() changes had been made save for this in this commit 6782b14

I still prefer to use PyDict_SetItemString().

IMO it's suitable for this situation, PyModule_AddObject() is more suitable for adding an object that doesn't need to be held by other variable.

I have another problem.
In issue24011's signalmodule.patch by Christian Heimes, there is a change:

+  finally:
     if (PyErr_Occurred()) {
         Py_DECREF(m);
         m = NULL;
     }
 
-  finally:
     return m;
 }

But this change has not been adopted, is it intentional or negligent? @tiran

I don't think that PyModule_AddIntMacro() can be used in this PR. I like the removal of "x" variable, and reuse DefaultHandler, IgnoreHandler and ItimerError.

@nanjekyejoannah: PyModule_AddObject() has terrible API, it's easy to misuse it...

@vstinner @animalize agreed.

If the PR looks good to you, would you mind to use GitHub UI to approve the PR?

@animalize: If you want to use PyDict_SetItemString(), please open a separated PR so I can compare the two approaches. Using PyDict_SetItemString() sounds like a good idea: internally, PyModule_AddObject calls PyDict_SetItemString, but in PyInit__signal() we already have have "d" (module dictionary).

In terms of fixing the current regression, this LGTM.

@animalize: If you want to use PyDict_SetItemString(), please open a separated PR so I can compare the two approaches. Using PyDict_SetItemString() sounds like a good idea: internally, PyModule_AddObject calls PyDict_SetItemString, but in PyInit__signal() we already have have "d" (module dictionary).

Please see this PR, it uses PyDict_SetItemString: #15753

I merged PR #15753 which is simpler than this PR.