-
Notifications
You must be signed in to change notification settings - Fork 747
Fix exception pickling #286
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
Conversation
The "args" slot of BaseException was not filled, instead we provided the args through our __getattr__ implementation. This fails in BaseException_reduce which depends on "args" being not NULL. We fix this by explicitly setting the "args" slot on all CLR exception objects on creation. This also makes tp_repr obsolete.
2537408
to
54868ec
Compare
@vmuriart @denfromufa @tonyroberts Please review. |
@filmor can you add tests compatible with both python 2 and 3 that use cPickle:
|
@denfromufa Done. Please check :) |
So is this a "feature" or "bug fix"? If latter, then let's merge this for Have you ran the embedding tests to make sure that exceptions do not break On Wed, Nov 9, 2016 at 4:22 AM, Benedikt Reinartz notifications@github.com
|
I haven't run the embedded tests, I'll check how to do that. This is definitely a bug fix, and a pretty critical one from my point of view (as said, this was killing my worker processes in production). |
Ok, please let me know if you have any luck running embedding tests and BTW, why do you pickle exceptions - really seems a wild idea to me? On Wed, Nov 9, 2016 at 9:20 AM, Benedikt Reinartz notifications@github.com
|
The tests work against Python 3.5 (though they have various issues, I'll prepare a PR). I pass the exceptions back from a multiprocessing worker to show them to the user, and during that they are automatically pickled. |
Fixes #284.
This PR contains 3 componenents:
BaseException
object instead of faking the functionality by overloading__getattr__
__getattr__
overload altogether since the only functionality it provides (lookup of.message
) is deprecated in Python >= 2.6