bpo-31356: Fixing PyErr_WarnEx might error out. #5456
Conversation
|
@lisroach A possible test for this is repeat warnings.filterwarnings('error')and then checking for a ======================================================================
ERROR: test_ensure_disabled_thread (Lib.test.test_gc.GCTogglingTests)
----------------------------------------------------------------------
RuntimeWarning: Garbage collector enabled while another thread is inside gc.ensure_enabled
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
File "/home/pgalindo3/cpython/Lib/test/support/__init__.py", line 2083, in decorator
return func(*args)
File "/home/pgalindo3/cpython/Lib/test/test_gc.py", line 1063, in test_ensure_disabled_thread
inside_status_after_thread = gc.isenabled()
SystemError: <built-in method __exit__ of gc.ensure_disabled object at 0x7f0253d22de0> returned a result with an error seAnother posible test is checking that SystemError is not raised / in stderr. |
| @@ -1068,8 +1068,10 @@ gc_enable_impl(PyObject *module) | |||
| /*[clinic end generated code: output=45a427e9dce9155c input=81ac4940ca579707]*/ | |||
| { | |||
| if(_PyRuntime.gc.disabled_threads){ | |||
| 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 " | |||
There was a problem hiding this comment.
PEP 7: Add a space between if and the opening parenthesis. Fix also a line above.
PEP 7: The line is too long (88 columns). It would be better to split it after the first argument.
| 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) == -1) { |
There was a problem hiding this comment.
PEP 7: Add a space after comma.
| @@ -1068,8 +1068,10 @@ gc_enable_impl(PyObject *module) | |||
| /*[clinic end generated code: output=45a427e9dce9155c input=81ac4940ca579707]*/ | |||
| { | |||
| if(_PyRuntime.gc.disabled_threads){ | |||
There was a problem hiding this comment.
PEP 7: Add a space between the closing parenthesis and the opening brace.
| "thread is inside gc.ensure_enabled",1); | ||
| if(PyErr_WarnEx(PyExc_RuntimeWarning, "Garbage collector enabled while another " | ||
| "thread is inside gc.ensure_enabled",1) == -1) { | ||
| PyErr_WriteUnraisable(module); |
| if(PyErr_WarnEx(PyExc_RuntimeWarning, "Garbage collector enabled while another " | ||
| "thread is inside gc.ensure_enabled",1) == -1) { | ||
| PyErr_WriteUnraisable(module); | ||
| } |
There was a problem hiding this comment.
This brace looks misaligned.
|
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 |
| 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) == -1) { |
There was a problem hiding this comment.
Since the condition is multiline, put the opening brace on a separate line.
|
@serhiy-storchaka @lisroach I looked at the crash and there are multiple places in the code that need to be fixed that can't be directly commented on in this PR. I'll open a new one. |
Fixing a potential bug found by Yury, if for whatever reason raising the warning were to fail we need to catch that.
I am not entirely sure how to best test this- I am open to suggestions anyone has!
https://bugs.python.org/issue31356