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

1st1 · 2018-01-31T16:14:44Z

https://bugs.python.org/issue31356

1st1 · 2018-01-31T16:16:14Z

1st1 · 2018-01-31T16:19:42Z

@ned-deily If there's time, when this one is reviewed by @lisroach and @serhiy-storchaka it would be great if it gets merged into beta-1.

Currently, running $ ./python -We -m test test_gc SIGABRTs on master.

vstinner · 2018-01-31T16:35:22Z

Modules/gcmodule.c

+
+    if (ret == NULL) {
+        return NULL;
+    }


Don't you have to DECREF somewhere? Try: ./python -m test -R 3:3 test_gc

Good catch; fixed.

vstinner · 2018-01-31T16:36:56Z

cc @pablogsal

1st1 · 2018-01-31T16:43:02Z

I don't understand the "test_ensure_disabled_thread" test: why is thread_inside_status is True?

1st1 · 2018-01-31T16:55:09Z

Why are we using PyGILState_Release in the code?

1st1 · 2018-01-31T17:05:01Z

Modules/gcmodule.c

@@ -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);


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

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.

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

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

IMO this is just plain wrong.

serhiy-storchaka · 2018-01-31T16:57:43Z

Lib/test/test_gc.py

-            thread = threading.Thread(target=disabling_thread)
-            thread.start()
-            inside_status_after_thread = gc.isenabled()
+        with self.assertWarnsRegex(RuntimeWarning, "enabled while another"):


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.

serhiy-storchaka · 2018-01-31T16:59:21Z

Modules/gcmodule.c

@@ -1068,8 +1068,13 @@ gc_enable_impl(PyObject *module)
 /*[clinic end generated code: output=45a427e9dce9155c input=81ac4940ca579707]*/
 {
    if(_PyRuntime.gc.disabled_threads){


Please fix the style of this line too.

serhiy-storchaka · 2018-01-31T17:09:13Z

Modules/gcmodule.c

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


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

serhiy-storchaka · 2018-01-31T17:12:42Z

Modules/gcmodule.c

@@ -1068,8 +1068,13 @@ gc_enable_impl(PyObject *module)
 /*[clinic end generated code: output=45a427e9dce9155c input=81ac4940ca579707]*/
 {
    if(_PyRuntime.gc.disabled_threads){


Please fix the style of this line too.

serhiy-storchaka · 2018-01-31T17:12:47Z

Modules/gcmodule.c

+    if (ret == NULL) {
+        return NULL;
+    }
+    Py_DECREF(ret);


You could just return ret.

ned-deily · 2018-01-31T17:15:01Z

If there's time, when this one is reviewed by @lisroach and @serhiy-storchaka it would be great if it gets merged into beta-1.

@1st1 Sorry, 3.7.0b1 is finished. Unless there is a consensus that this needs an emergency fix because 3.7.0b1 is unusable, it will have to wait for the scheduled release of 3.7.0b2 in 4 weeks.

gpshead · 2018-01-31T17:16:37Z

Modules/gcmodule.c

@@ -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);


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.

bedevere-bot · 2018-01-31T17:17:07Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

1st1 · 2018-01-31T17:19:27Z

I don't understand the "test_ensure_disabled_thread" test: why is thread_inside_status is True?

@serhiy-storchaka @gpshead Can you explain this ^^?

1st1 · 2018-01-31T17:27:20Z

@1st1 Sorry, 3.7.0b1 is finished. Unless there is a consensus that this needs an emergency fix because 3.7.0b1 is unusable, it will have to wait for the scheduled release of 3.7.0b2 in 4 weeks.

@ned-deily Can you run $ ./python -We -m test test_gc? It sig-aborts in debug build. I don't know if that's acceptable or not, your call.

1st1 · 2018-01-31T17:41:40Z

Moreover, currently with gc.ensure_disabled() releases GIL for any Python code run inside it. So it looks like that this is completely broken.

ned-deily · 2018-01-31T17:51:33Z

@1st1 The trap abort is unfortunate but it seems like that it only happens on a debug build which non-developer users will not see. The question is what impact does this bug have on users. Can we please move that discussion to bpo-31356 to get a wider audience?

1st1 · 2018-01-31T18:01:24Z

@ned-deily Sure. See new comments from Serhiy and me in the issue.

1st1 · 2018-01-31T20:25:46Z

Closing this one as it was decided that we'll simply revert the original PR instead of fixing it. Thanks!

the-knights-who-say-ni added the CLA signed label Jan 31, 2018

bedevere-bot added the awaiting merge label Jan 31, 2018

1st1 mentioned this pull request Jan 31, 2018

bpo-31356: Fixing PyErr_WarnEx might error out. #5456

Closed

1st1 requested a review from serhiy-storchaka January 31, 2018 16:15

1st1 added the skip news label Jan 31, 2018

1st1 force-pushed the fix_31356 branch from 1f75272 to ab8a66a Compare January 31, 2018 16:20

1st1 requested a review from vstinner January 31, 2018 16:30

vstinner reviewed Jan 31, 2018

View reviewed changes

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

2117784

1st1 force-pushed the fix_31356 branch from ab8a66a to 2117784 Compare January 31, 2018 16:36

1st1 commented Jan 31, 2018

View reviewed changes

serhiy-storchaka approved these changes Jan 31, 2018

View reviewed changes

gpshead requested changes Jan 31, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Jan 31, 2018

1st1 closed this Jan 31, 2018

1st1 deleted the fix_31356 branch January 31, 2018 20:28

rhettinger mentioned this pull request May 20, 2023

Add context manager to temporarily disable GC #75537

Open

Uh oh!

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

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

Uh oh!

Conversation

1st1 commented Jan 31, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

1st1 commented Jan 31, 2018

Uh oh!

1st1 commented Jan 31, 2018 • edited 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!

vstinner commented Jan 31, 2018

Uh oh!

1st1 commented Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

1st1 commented Jan 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1st1 Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ned-deily commented Jan 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jan 31, 2018

Uh oh!

1st1 commented Jan 31, 2018

Uh oh!

1st1 commented Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

1st1 commented Jan 31, 2018

Uh oh!

ned-deily commented Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

1st1 commented Jan 31, 2018

Uh oh!

1st1 commented Jan 31, 2018

Uh oh!

Uh oh!

1st1 commented Jan 31, 2018 •

edited by bedevere-bot

Loading

1st1 commented Jan 31, 2018 •

edited

Loading

1st1 commented Jan 31, 2018 •

edited

Loading

1st1 Jan 31, 2018 •

edited

Loading

1st1 commented Jan 31, 2018 •

edited

Loading

ned-deily commented Jan 31, 2018 •

edited

Loading