Skip to content
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.

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

SystemError with bare raise in threading or multiprocessing #71745

Closed
romuald mannequin opened this issue Jul 18, 2016 · 24 comments
Closed

SystemError with bare raise in threading or multiprocessing #71745

romuald mannequin opened this issue Jul 18, 2016 · 24 comments
Assignees
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@romuald
Copy link
Mannequin

romuald mannequin commented Jul 18, 2016

BPO 27558
Nosy @rhettinger, @pitrou, @vstinner, @bitdancer, @berkerpeksag, @ztane, @romuald, @applio, @zhangyangyu, @Vgr255
Files
  • example.py: Reproducible example
  • issue27558.patch
  • issue27558_v2.patch: add test in test_threading instead of test_raise
  • issue27558_v3.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/applio'
    closed_at = <Date 2016-08-18.16:24:42.283>
    created_at = <Date 2016-07-18.06:26:43.558>
    labels = ['interpreter-core', 'type-bug']
    title = 'SystemError with bare `raise` in threading or multiprocessing'
    updated_at = <Date 2016-08-19.07:20:26.970>
    user = 'https://github.com/romuald'

    bugs.python.org fields:

    activity = <Date 2016-08-19.07:20:26.970>
    actor = 'SilentGhost'
    assignee = 'davin'
    closed = True
    closed_date = <Date 2016-08-18.16:24:42.283>
    closer = 'vstinner'
    components = ['Interpreter Core']
    creation = <Date 2016-07-18.06:26:43.558>
    creator = 'Romuald'
    dependencies = []
    files = ['43772', '43776', '43971', '44137']
    hgrepos = []
    issue_num = 27558
    keywords = ['patch']
    message_count = 24.0
    messages = ['270713', '270728', '270729', '270730', '270733', '270734', '270735', '270746', '270755', '270757', '270759', '270761', '270763', '271802', '271822', '272407', '272409', '272436', '272938', '272943', '273011', '273043', '273044', '273046']
    nosy_count = 11.0
    nosy_names = ['rhettinger', 'pitrou', 'vstinner', 'r.david.murray', 'python-dev', 'berker.peksag', 'ztane', 'Romuald', 'davin', 'xiang.zhang', 'abarry']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue27558'
    versions = ['Python 3.5', 'Python 3.6']

    @romuald
    Copy link
    Mannequin Author

    romuald mannequin commented Jul 18, 2016

    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

    @romuald romuald mannequin added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels Jul 18, 2016
    @ztane
    Copy link
    Mannequin

    ztane mannequin commented Jul 18, 2016

    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

    @ztane
    Copy link
    Mannequin

    ztane mannequin commented Jul 18, 2016

    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.

    @ztane
    Copy link
    Mannequin

    ztane mannequin commented Jul 18, 2016

    more easily reproducible by

        import threading
        def foo():
            raise
        threading.Thread(target=foo).start()

    @zhangyangyu
    Copy link
    Member

    It seems the thread state is initialized in thread_PyThread_start_new_thread. It calls _PyThreadState_Prealloc which initialize exc_type to NULL.

    @zhangyangyu
    Copy link
    Member

    I am curious why individual raise doesn't get the same problem in single thread program.

    @ztane
    Copy link
    Mannequin

    ztane mannequin commented Jul 18, 2016

    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.

    @zhangyangyu
    Copy link
    Member

    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.

    @Vgr255 Vgr255 mannequin changed the title SystemError inside multiprocessing.dummy Pool.map SystemError with bare raise in threading or multiprocessing Jul 18, 2016
    @zhangyangyu
    Copy link
    Member

    Upload a patch.

    @berkerpeksag
    Copy link
    Member

    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.

    @zhangyangyu
    Copy link
    Member

    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.

    @bitdancer
    Copy link
    Member

    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.

    @zhangyangyu
    Copy link
    Member

    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.

    @zhangyangyu
    Copy link
    Member

    Update the patch with Berker's suggestion: add test in test_threading instead of test_raise. Hope someone is willing to merge this.

    @berkerpeksag
    Copy link
    Member

    The test code in issue27558_v2.patch looks OK to me.

    @zhangyangyu
    Copy link
    Member

    Ping. Mind to merge?

    @rhettinger
    Copy link
    Contributor

    Davin, would you like to finish this one (NEWS and ACKS entries) and get it committed?

    @berkerpeksag
    Copy link
    Member

    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.

    @vstinner
    Copy link
    Member

    I reviewed issue27558_v2.patch, see my comments.

    @zhangyangyu
    Copy link
    Member

    Thanks for your review, Victor. :) Leave a reply.

    @zhangyangyu
    Copy link
    Member

    Victor, upload a new patch, changing the test case to the approach you prefer. ;)

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Aug 18, 2016

    New changeset ba45fbb16499 by Victor Stinner in branch '3.5':
    Fix SystemError in "raise" statement
    https://hg.python.org/cpython/rev/ba45fbb16499

    @vstinner
    Copy link
    Member

    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.

    @zhangyangyu
    Copy link
    Member

    Thanks for your work too! ;) I agree to close.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants