bpo-32696 Fixed pickling exceptions with multiple arguments #30602
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA). Recognized GitHub usernameWe couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames: This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. CLA MissingOur records indicate the following people have not signed the CLA: For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. If you have recently signed the CLA, please wait at least one business day You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
Lib/test/_test_multiprocessing.py
Outdated
| @@ -2744,28 +2741,6 @@ def errback(exc): | |||
| p.close() | |||
| p.join() | |||
|
|
|||
| def test_unpickleable_result(self): | |||
There was a problem hiding this comment.
Can you comment on why this seemingly unrelated test had to be removed?
There was a problem hiding this comment.
Because, newargs unpickable. BaseException_reduce return newargs instead of args if newargs != NULL. If super init was called, it takes args from locals (of init function) and save to newargs. In this test "lambda" in newargs (this test could be passed before, because of modified args equal to ("string",)), so it throws an error and crash test_multiprocessing_spawn.
There was a problem hiding this comment.
So which part of this test was failing? What you're saying suggests that this change breaks backward compatibility, which is concerning.
There was a problem hiding this comment.
Failing pickle lambda function. I can add check in BaseException_reduce if (lambda not in newargs) return newargs else return args. Then this test can be passed.
Lib/test/_test_multiprocessing.py
Outdated
| @@ -2744,28 +2741,6 @@ def errback(exc): | |||
| p.close() | |||
| p.join() | |||
|
|
|||
| def test_unpickleable_result(self): | |||
There was a problem hiding this comment.
So which part of this test was failing? What you're saying suggests that this change breaks backward compatibility, which is concerning.
Objects/typeobject.c
Outdated
| @@ -9045,6 +9045,26 @@ super_init(PyObject *self, PyObject *args, PyObject *kwds) | |||
| return -1; | |||
| Py_INCREF(obj); | |||
| } | |||
|
|
|||
| if (PyType_Check(type) && PyObject_IsSubclass((PyObject*)type, PyExc_BaseException) && PyObject_IsInstance(obj, (PyObject*)type)) { | |||
There was a problem hiding this comment.
PyObject_IsSubclass() can throw an error, in which case it returns -1. This needs to be checked. Many other calls below are also missing error checks.
More broadly it seems wrong to have BaseException-specific code here, and it is fragile to check the flags of the code object directly here. I think you need to go back and come up with a more robust design for this feature.
…eption_newargs_superinit
| @@ -7,7 +7,7 @@ | |||
| /* PyException_HEAD defines the initial segment of every exception class. */ | |||
| #define PyException_HEAD PyObject_HEAD PyObject *dict;\ | |||
| PyObject *args; PyObject *note; PyObject *traceback;\ | |||
| PyObject *context; PyObject *cause;\ | |||
| PyObject *context; PyObject* newargs; PyObject *cause;\ | |||
There was a problem hiding this comment.
You can't do this. You're creating references to all the args, which may cause memory leaks. This may well be the reason that the programmer is not keeping references to the args but deriving something from them.
|
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 And if you don't make the requested changes, you will be put in the comfy chair! |
https://bugs.python.org/issue32696
https://bugs.python.org/issue32696