bpo-38266: Revert bpo-37878: Make PyThreadState_DeleteCurrent() Internal by nanjekyejoannah · Pull Request #16558 · python/cpython

nanjekyejoannah · 2019-10-03T12:52:17Z

Reverting the removal of PyThreadState_DeleteCurrent() with documentation.

https://bugs.python.org/issue38266

vstinner · 2019-10-03T20:55:34Z

Doc/c-api/init.rst

+    Destroy the current thread state and release the global interpreter lock.
+    Like :c:func:`PyThreadState_Delete`, the global interpreter lock need not
+    be held. The thread state must have been reset with a previous call
+    to :c:func:`PyThreadState_Clear`.


If we start to "expose" this function, would it be possible to be less strict and call PyThreadState_Clear() in PyThreadState_DeleteCurrent()? It sems like calling PyThreadState_Clear() twice is safe.

I marked https://bugs.python.org/issue38266 as a release blocker, so the PyThreadState_Clear() question can definitely wait after 3.8.0: we can even revisit this in Python 3.9. Let's stick to adding back the function unmodified.

vstinner · 2019-10-03T20:56:51Z

Include/internal/pycore_pylifecycle.h

 PyAPI_FUNC(void) _PyErr_Display(PyObject *file, PyObject *exception,
                                PyObject *value, PyObject *tb);

-PyAPI_FUNC(void) _PyThreadState_DeleteCurrent(_PyRuntimeState *runtime);


I don't understand why do you remove this definition: it's still used, no?

I am now exposing PyThreadState_DeleteCurrent() instead of the internal _PyThreadState_DeleteCurrent() I thought. what are you thinking?

_PyThreadState_DeleteCurrent() has to stay, it's part of https://pythoncapi.readthedocs.io/runtime.html effort to avoid global variables.

vstinner · 2019-10-03T20:57:10Z

Include/cpython/pystate.h

 PyAPI_FUNC(PyInterpreterState *) PyInterpreterState_Next(PyInterpreterState *);
 PyAPI_FUNC(PyThreadState *) PyInterpreterState_ThreadHead(PyInterpreterState *);
 PyAPI_FUNC(PyThreadState *) PyThreadState_Next(PyThreadState *);
+PyAPI_FUNC(void) PyThreadState_DeleteCurrent();


The prototype must be:

Suggested change

PyAPI_FUNC(void) PyThreadState_DeleteCurrent();

PyAPI_FUNC(void) PyThreadState_DeleteCurrent(void);

vstinner · 2019-10-03T20:58:26Z

Modules/_tracemalloc.c

 /* This lock is needed because tracemalloc_free() is called without
   the GIL held from PyMem_RawFree(). It cannot acquire the lock because it
-   would introduce a deadlock in _PyThreadState_DeleteCurrent(). */
+   would introduce a deadlock in PyThreadState_DeleteCurrent(). */


I prefer to keep _PyThreadState_DeleteCurrent() here. PyThreadState_DeleteCurrent() is only a public helper function, _PyThreadState_DeleteCurrent() is the real implementation.

vstinner · 2019-10-03T20:58:39Z

Modules/_tracemalloc.c


     /* GIL cannot be locked in PyMem_RawFree() because it would introduce
-        a deadlock in _PyThreadState_DeleteCurrent(). */
+        a deadlock in PyThreadState_DeleteCurrent(). */


ditto: keep _PyThreadState_DeleteCurrent()

vstinner · 2019-10-03T20:59:25Z

Python/pystate.c

 }

+void
+PyThreadState_DeleteCurrent()


Please add void to prevent accepting any argument:

Suggested change

PyThreadState_DeleteCurrent()

PyThreadState_DeleteCurrent(void)

vstinner · 2019-10-03T21:00:10Z

Once this change lands, it would be interested to add an unit test, but I'm not sure how to write such test. C code in _testcapi I guess.

nanjekyejoannah · 2019-10-03T23:59:21Z

@vstinner , I have addressed the changes. PTAL

vstinner

LGTM.

vstinner · 2019-10-04T00:12:21Z

Doc/c-api/init.rst

+    Destroy the current thread state and release the global interpreter lock.
+    Like :c:func:`PyThreadState_Delete`, the global interpreter lock need not
+    be held. The thread state must have been reset with a previous call
+    to :c:func:`PyThreadState_Clear`.


I marked https://bugs.python.org/issue38266 as a release blocker, so the PyThreadState_Clear() question can definitely wait after 3.8.0: we can even revisit this in Python 3.9. Let's stick to adding back the function unmodified.

vstinner · 2019-10-04T00:13:22Z

Include/internal/pycore_pylifecycle.h

 PyAPI_FUNC(void) _PyErr_Display(PyObject *file, PyObject *exception,
                                PyObject *value, PyObject *tb);

-PyAPI_FUNC(void) _PyThreadState_DeleteCurrent(_PyRuntimeState *runtime);


_PyThreadState_DeleteCurrent() has to stay, it's part of https://pythoncapi.readthedocs.io/runtime.html effort to avoid global variables.

vstinner · 2019-10-04T00:14:38Z

Include/internal/pycore_pylifecycle.h

 PyAPI_FUNC(void) _PyErr_Display(PyObject *file, PyObject *exception,
-                                PyObject *value, PyObject *tb);
-
+                                PyObject *value, PyObject *tb);                           


nitpick: it seems like you removed a newline or something else. you may revert that.

vstinner · 2019-10-04T00:18:34Z

According to https://bugs.python.org/issue38266 discussion, it's a Python 3.8 regression and so this change must be backported to 3.8. I suggest to merge this change into 3.8 and master as soon as possible, to attempt to get it into 3.8.0 final. (But it's ok if we miss the final, it can wait for 3.8.1 otherwise.)

vstinner

LGTM.

vstinner · 2019-10-04T11:35:11Z

Usually, we let core developers merge their own pull requests. But. Joannah explicitly asked me to merge her PR if I want. Since this change fix a Python 3.8 regression and the next 3.8.0 release (which may be a rc2 or the final) is in 3 days, I prefer to merge it ASAP.

miss-islington · 2019-10-04T11:35:44Z

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

miss-islington · 2019-10-04T11:35:46Z

I'm having trouble backporting to 3.8. Reason: 'Error 110 while writing to socket. Connection timed out.'. Please retry by removing and re-adding the needs backport to 3.8 label.

miss-islington · 2019-10-04T11:36:00Z

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

miss-islington · 2019-10-04T11:36:05Z

Sorry, @nanjekyejoannah and @vstinner, I could not cleanly backport this to 3.8 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 8855e47d09d4db1206c65b24efc8ad0df585ac46 3.8

miss-islington · 2019-10-11T21:56:26Z

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

miss-islington · 2019-10-11T21:56:30Z

Sorry, @nanjekyejoannah and @vstinner, I could not cleanly backport this to 3.8 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 8855e47d09d4db1206c65b24efc8ad0df585ac46 3.8

…nal (pythonGH-16558) Revert the removal of PyThreadState_DeleteCurrent() with documentation.

Revert bpo-37878: Make PyThreadState_DeleteCurrent() Internal

b680b9a

nanjekyejoannah requested a review from vstinner October 3, 2019 12:52

the-knights-who-say-ni added the CLA signed label Oct 3, 2019

bedevere-bot added the awaiting core review label Oct 3, 2019

📜🤖 Added by blurb_it.

7649d8d

nanjekyejoannah requested a review from ericsnowcurrently October 3, 2019 12:55

vstinner reviewed Oct 3, 2019

View reviewed changes

Revert bpo-37878: Make PyThreadState_DeleteCurrent() Internal

7acac6f

vstinner approved these changes Oct 4, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Oct 4, 2019

vstinner reviewed Oct 4, 2019

View reviewed changes

vstinner added the needs backport to 3.8 label Oct 4, 2019

nanjekyejoannah added 3 commits October 3, 2019 21:19

remove unneccessary space diff

0c1a14d

Remove space

c54ccac

Remove space

3310409

vstinner approved these changes Oct 4, 2019

View reviewed changes

vstinner merged commit 8855e47 into python:master Oct 4, 2019

bedevere-bot removed the awaiting merge label Oct 4, 2019

vstinner added needs backport to 3.8 and removed needs backport to 3.8 labels Oct 4, 2019

miss-islington assigned vstinner Oct 4, 2019

zooba added needs backport to 3.8 and removed needs backport to 3.8 labels Oct 11, 2019

jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019

bpo-38266: Revert bpo-37878: Make PyThreadState_DeleteCurrent() Inter…

8958043

…nal (pythonGH-16558) Revert the removal of PyThreadState_DeleteCurrent() with documentation.

bstaletic mentioned this pull request May 26, 2020

Pybind11 doesn't compile with CPython 3.9b1 pybind/pybind11#2224

Closed

	PyAPI_FUNC(void) PyThreadState_DeleteCurrent();
	PyAPI_FUNC(void) PyThreadState_DeleteCurrent(void);

	PyThreadState_DeleteCurrent()
	PyThreadState_DeleteCurrent(void)

Uh oh!

Comments

Conversation

nanjekyejoannah commented Oct 3, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Oct 3, 2019

Uh oh!

nanjekyejoannah commented Oct 3, 2019

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Oct 4, 2019

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Oct 4, 2019

Uh oh!

miss-islington commented Oct 4, 2019

Uh oh!

miss-islington commented Oct 4, 2019

Uh oh!

miss-islington commented Oct 4, 2019

Uh oh!

miss-islington commented Oct 4, 2019

Uh oh!

miss-islington commented Oct 11, 2019

Uh oh!

miss-islington commented Oct 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nanjekyejoannah commented Oct 3, 2019 •

edited by bedevere-bot

Loading