Skip to content

bpo-31356: Fix multiple errors in gc.ensure_disabled() #5462

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

bpo-31356: Fix multiple errors in gc.ensure_disabled() #5462

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 6 additions & 9 deletions Lib/test/test_gc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1054,19 +1054,16 @@ def disabling_thread():

original_status = gc.isenabled()

with warnings.catch_warnings(record=True) as w, gc.ensure_disabled():
inside_status_before_thread = gc.isenabled()
thread = threading.Thread(target=disabling_thread)
thread.start()
inside_status_after_thread = gc.isenabled()
with self.assertWarnsRegex(RuntimeWarning, "enabled while another"):
Copy link
Member

Choose a reason for hiding this comment

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

What line emits a warning? It would be better to surround with assertWarnsRegex() only that line. Or it can be emitted by arbitrary line in the block?

Note that warnings emitted in other thread are not caught here.

with gc.ensure_disabled():
inside_status_before_thread = gc.isenabled()
thread = threading.Thread(target=disabling_thread)
thread.start()
inside_status_after_thread = gc.isenabled()

after_status = gc.isenabled()
thread.join()

self.assertEqual(len(w), 1)
self.assertTrue(issubclass(w[-1].category, RuntimeWarning))
self.assertEqual("Garbage collector enabled while another thread is "
"inside gc.ensure_enabled", str(w[-1].message))
self.assertEqual(original_status, True)
self.assertEqual(inside_status_before_thread, False)
self.assertEqual(thread_original_status, False)
Expand Down
36 changes: 28 additions & 8 deletions Modules/gcmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1068,8 +1068,13 @@ gc_enable_impl(PyObject *module)
/*[clinic end generated code: output=45a427e9dce9155c input=81ac4940ca579707]*/
{
if(_PyRuntime.gc.disabled_threads){
Copy link
Member

Choose a reason for hiding this comment

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

Please fix the style of this line too.

Copy link
Member

Choose a reason for hiding this comment

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

Please fix the style of this line too.

PyErr_WarnEx(PyExc_RuntimeWarning, "Garbage collector enabled while another "
"thread is inside gc.ensure_enabled",1);
if (PyErr_WarnEx(
PyExc_RuntimeWarning,
"Garbage collector enabled while another "
"thread is inside gc.ensure_enabled", 1) < 0)
{
return NULL;
}
}
_PyRuntime.gc.enabled = 1;
Py_RETURN_NONE;
Expand Down Expand Up @@ -1530,8 +1535,13 @@ ensure_disabled__enter__method(ensure_disabled_object *self, PyObject *args)
PyGILState_STATE gstate = PyGILState_Ensure();
++_PyRuntime.gc.disabled_threads;
self->previous_gc_state = _PyRuntime.gc.enabled;
gc_disable_impl(NULL);

PyObject *ret = gc_disable_impl(NULL);
PyGILState_Release(gstate);
Copy link
Member Author

Choose a reason for hiding this comment

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

@serhiy-storchaka Do you understand this PyGILState_Release call? Isn't it unsafe to execute Python/CAPI code after it?

Copy link
Member

Choose a reason for hiding this comment

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

here and below, the

    PyGILState_Release(gstate);
    if (ret == NULL) {
        return NULL;
    }
    Py_DECREF(ret);

blocks should be replaced with

    Py_XDECREF(ret);
    PyGILState_Release(gstate);
    if (ret == NULL) {
        return NULL;
    }

The GIL must be held for the decref.

Copy link
Member Author

@1st1 1st1 Jan 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

But this is a Python API! So we release the gil inside with ensure_disabled(). Why?

Copy link
Member

Choose a reason for hiding this comment

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

I didn't review the initial changes and know nothing about the correctness of other code. I'll check it carefully after beta1.

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO this is just plain wrong.

if (ret == NULL) {
return NULL;
}
Py_DECREF(ret);
Copy link
Member

Choose a reason for hiding this comment

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

You could just return ret.

Py_RETURN_NONE;
}

Expand All @@ -1540,12 +1550,19 @@ ensure_disabled__exit__method(ensure_disabled_object *self, PyObject *args)
{
PyGILState_STATE gstate = PyGILState_Ensure();
--_PyRuntime.gc.disabled_threads;
if(self->previous_gc_state){
gc_enable_impl(NULL);
}else{
gc_disable_impl(NULL);

PyObject *ret;
if (self->previous_gc_state) {
ret = gc_enable_impl(NULL);
}
else {
ret = gc_disable_impl(NULL);
}
PyGILState_Release(gstate);
if (ret == NULL) {
return NULL;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it would be better to chain exceptions, but this is an additional non-trivial code. We can do this in beta2.

}
Py_DECREF(ret);
Py_RETURN_NONE;
}

Expand Down Expand Up @@ -1650,8 +1667,11 @@ PyInit_gc(void)

if (PyType_Ready(&gc_ensure_disabled_type) < 0)
return NULL;
if (PyModule_AddObject(m, "ensure_disabled", (PyObject*) &gc_ensure_disabled_type) < 0)
if (PyModule_AddObject(m, "ensure_disabled",
(PyObject*) &gc_ensure_disabled_type) < 0)
{
return NULL;
}


#define ADD_INT(NAME) if (PyModule_AddIntConstant(m, #NAME, NAME) < 0) return NULL
Expand Down