classification
Title: SystemError with bare `raise` in threading or multiprocessing
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.6, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: davin Nosy List: Romuald, abarry, berker.peksag, davin, pitrou, python-dev, r.david.murray, rhettinger, vstinner, xiang.zhang, ztane
Priority: normal Keywords: patch

Created on 2016-07-18 06:26 by Romuald, last changed 2016-08-19 07:20 by SilentGhost. This issue is now closed.

Files
File name Uploaded Description Edit
example.py Romuald, 2016-07-18 06:26 Reproducible example
issue27558.patch xiang.zhang, 2016-07-18 13:38 review
issue27558_v2.patch xiang.zhang, 2016-08-02 04:48 add test in test_threading instead of test_raise review
issue27558_v3.patch xiang.zhang, 2016-08-18 04:14 review
Messages (24)
msg270713 - (view) Author: Romuald Brunet (Romuald) * Date: 2016-07-18 06:26
Raising without a previous exception inside a method called from multiprocessing.dummy.Pool.map will trigger a SystemError.

SystemError: PyEval_EvalFrameEx returned NULL without setting an error


Traceback (most recent call last):
  File "example.py", line 16, in <module>
    main()
  File "example.py", line 11, in main
    result = pool.map(test, [1])
  File "/usr/local/Cellar/python3/3.5.2/Frameworks/Python.framework/Versions/3.5/lib/python3.5/multiprocessing/pool.py", line 260, in map
    return self._map_async(func, iterable, mapstar, chunksize).get()
  File "/usr/local/Cellar/python3/3.5.2/Frameworks/Python.framework/Versions/3.5/lib/python3.5/multiprocessing/pool.py", line 608, in get
    raise self._value
  File "/usr/local/Cellar/python3/3.5.2/Frameworks/Python.framework/Versions/3.5/lib/python3.5/multiprocessing/pool.py", line 119, in worker
    result = (True, func(*args, **kwds))
  File "/usr/local/Cellar/python3/3.5.2/Frameworks/Python.framework/Versions/3.5/lib/python3.5/multiprocessing/pool.py", line 44, in mapstar
    return list(map(*args))
SystemError: PyEval_EvalFrameEx returned NULL without setting an error


Reproducible example is attached
msg270728 - (view) Author: Antti Haapala (ztane) * Date: 2016-07-18 08:51
Reproducible on Python 3.6a4ish on Ubuntu. I believe this needs forking multiprocessing.

do_raise is called with 2 NULLs as arguments, it should raise

    PyErr_SetString(PyExc_RuntimeError,
                            "No active exception to reraise");

What happens is that PyThreadState is initialized to *all* NULL pointers on the new thread on multiprocessing, however `type` is expected to point to `Py_None` when no exception has been raised:

        PyThreadState *tstate = PyThreadState_GET();
        PyObject *tb;
        type = tstate->exc_type;
        value = tstate->exc_value;
        tb = tstate->exc_traceback;
        if (type == Py_None) {
            PyErr_SetString(PyExc_RuntimeError,
                            "No active exception to reraise");
            return 0;
        }

I am not sure where the thread state should have been initialized though
msg270729 - (view) Author: Antti Haapala (ztane) * Date: 2016-07-18 09:06
OTOH, if you put sys.exc_info() in place of raise there, it correctly (None, None, None) there, because it does (sysmodule.c:sys_exc_info)

        tstate->exc_type != NULL ? tstate->exc_type : Py_None,

Easiest fix would be to make do_raise test for both NULL and None, but I'd really consider fixing the new thread initialization if possible.
msg270730 - (view) Author: Antti Haapala (ztane) * Date: 2016-07-18 09:07
more easily reproducible by

    import threading
    def foo():
        raise
    threading.Thread(target=foo).start()
msg270733 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-07-18 09:13
It seems the thread state is initialized in thread_PyThread_start_new_thread. It calls _PyThreadState_Prealloc which initialize exc_type to NULL.
msg270734 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-07-18 09:21
I am curious why individual `raise` doesn't get the same problem in single thread program.
msg270735 - (view) Author: Antti Haapala (ztane) * Date: 2016-07-18 09:25
I was thinking that perhaps an exception is always raised somewhere before? I tried skipping site, but it still works, so I am not too sure.
msg270746 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-07-18 10:41
> Easiest fix would be to make do_raise test for both NULL and None, but I'd really consider fixing the new thread initialization if possible.

I think the easiest fix is OK. The new thread initialization is consistent and should not be changed. The Py_None seems to be set in somewhere in ceval but I am not sure where. You can see at https://hg.python.org/cpython/file/tip/Python/ceval.c#l1200 both NULL and Py_None are checked.
msg270755 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-07-18 13:38
Upload a patch.
msg270757 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-07-18 14:06
I would prefer Antti's test case in msg270730 since the bug can be reproduced by using threading module (you can use assert_python_ok() as a helper.) Lib/test_threading.py is probably a better place for the test.
msg270759 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-07-18 14:36
It's reasonable but I'd still like to hear others' voices since it looks more like to me it's the behaviour of raise itself rather than threading that crashes.
msg270761 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-07-18 14:52
Since ceval already has such a check, I think we should fix it by adding the check as proposed.  Investigating whether the codebase could be simplified by making sure that Py_None is always there after startup would be a separate issue, I think, possibly related to PEP 432.
msg270763 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-07-18 15:30
A simple try ... except ... can set the variable to Py_None:

>>> import threading
>>> def foo():
...     try:
...             raise RuntimeError
...     except:
...             pass
...     raise
... 
>>> threading.Thread(target=foo).start()
>>> Exception in thread Thread-1:
Traceback (most recent call last):
  File "/usr/lib/python3.4/threading.py", line 920, in _bootstrap_inner
    self.run()
  File "/usr/lib/python3.4/threading.py", line 868, in run
    self._target(*self._args, **self._kwargs)
  File "<stdin>", line 6, in foo
RuntimeError: No active exception to reraise

In the startup procedure, some c-apis are called and there are patterns like:

try:
    import some_module
except ImportError:
    some_module = ...

So I think after startup the variable can't be NULL. But we do see a NULL case here.
msg271802 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-02 04:48
Update the patch with Berker's suggestion: add test in test_threading instead of test_raise. Hope someone is willing to merge this.
msg271822 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-08-02 13:51
The test code in issue27558_v2.patch looks OK to me.
msg272407 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-11 07:13
Ping. Mind to merge?
msg272409 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2016-08-11 07:25
Davin, would you like to finish this one (NEWS and ACKS entries) and get it committed?
msg272436 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-08-11 10:32
Raymond, please stop hijacking issues. This doesn't have anything to do with multiprocessing and there are already four core developers in the nosy list. If you really want to assign it to someone else, please at least wait for a month or ask if they have time to commit the patch first. Unless I'm missing something, this doesn't look like a release blocker to me and definitely doesn't look like some 6 years old ancient issue (the first message was posted three weeks ago) so there is no urgency here.
msg272938 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-08-17 13:25
I reviewed  issue27558_v2.patch, see my comments.
msg272943 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-17 13:57
Thanks for your review, Victor. :) Leave a reply.
msg273011 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-18 04:14
Victor, upload a new patch, changing the test case to the approach you prefer. ;)
msg273043 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-08-18 16:15
New changeset ba45fbb16499 by Victor Stinner in branch '3.5':
Fix SystemError in "raise" statement
https://hg.python.org/cpython/rev/ba45fbb16499
msg273044 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-08-18 16:19
Thanks Xiang Zhang. The fix is obvious and simple, but it wasn't easy to identify it ;-) I pushed the fix to Python 3.5 and default (3.6).

Python 2.7 doesn't crash, the bare "raise" statement raises the exception: TypeError('exceptions must be old-style classes or derived from BaseException, not NoneType',). I suggest to not touch Python 2.7 and close the issue.
msg273046 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2016-08-18 16:23
Thanks for your work too! ;) I agree to close.
History
Date User Action Args
2016-08-19 07:20:26SilentGhostsetstage: patch review -> resolved
2016-08-18 16:24:42vstinnersetstatus: open -> closed
resolution: fixed
2016-08-18 16:23:55xiang.zhangsetmessages: + msg273046
2016-08-18 16:19:18vstinnersetmessages: + msg273044
2016-08-18 16:15:15python-devsetnosy: + python-dev
messages: + msg273043
2016-08-18 04:14:23xiang.zhangsetfiles: + issue27558_v3.patch

messages: + msg273011
2016-08-17 13:57:55xiang.zhangsetmessages: + msg272943
2016-08-17 13:25:48vstinnersetmessages: + msg272938
2016-08-11 10:32:22berker.peksagsetmessages: + msg272436
2016-08-11 07:25:36rhettingersetassignee: davin

messages: + msg272409
nosy: + rhettinger
2016-08-11 07:13:17xiang.zhangsetmessages: + msg272407
2016-08-02 13:51:17berker.peksagsetmessages: + msg271822
2016-08-02 04:48:29xiang.zhangsetfiles: + issue27558_v2.patch

messages: + msg271802
2016-07-18 15:30:02xiang.zhangsetmessages: + msg270763
2016-07-18 14:52:00r.david.murraysetnosy: + r.david.murray
messages: + msg270761
2016-07-18 14:36:43xiang.zhangsetmessages: + msg270759
2016-07-18 14:28:47vstinnersetnosy: + vstinner
2016-07-18 14:06:20berker.peksagsetversions: - Python 3.4
nosy: + berker.peksag

messages: + msg270757

stage: needs patch -> patch review
2016-07-18 13:38:26xiang.zhangsetfiles: + issue27558.patch
keywords: + patch
messages: + msg270755
2016-07-18 11:06:02abarrysetnosy: + pitrou, davin, abarry
title: SystemError inside multiprocessing.dummy Pool.map -> SystemError with bare `raise` in threading or multiprocessing

stage: needs patch
versions: + Python 3.6, - Python 3.2, Python 3.3
2016-07-18 10:41:34xiang.zhangsetmessages: + msg270746
2016-07-18 09:25:18ztanesetmessages: + msg270735
2016-07-18 09:21:15xiang.zhangsetmessages: + msg270734
2016-07-18 09:13:56xiang.zhangsetmessages: + msg270733
2016-07-18 09:07:47ztanesetmessages: + msg270730
2016-07-18 09:06:24ztanesetmessages: + msg270729
2016-07-18 08:51:34ztanesetnosy: + ztane
messages: + msg270728
2016-07-18 06:35:32xiang.zhangsetnosy: + xiang.zhang
2016-07-18 06:26:43Romualdcreate