Skip to content

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

Merged
merged 7 commits into from
Nov 10, 2016
Merged

Conversation

filmor
Copy link
Member

@filmor filmor commented Nov 8, 2016

Fixes #284.

This PR contains 3 componenents:

  1. Remove all of the Py<2.5 workarounds that wrap exceptions in old-style class instances
  2. Fix Trying to pickle an exception crashes the interpreter #284 by moving the exception args into the corresponding slot in the BaseException object instead of faking the functionality by overloading __getattr__
  3. Remove the __getattr__ overload altogether since the only functionality it provides (lookup of .message) is deprecated in Python >= 2.6

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.
@filmor filmor force-pushed the fix-exception-pickling branch from 2537408 to 54868ec Compare November 8, 2016 14:06
@filmor
Copy link
Member Author

filmor commented Nov 8, 2016

@vmuriart @denfromufa @tonyroberts Please review.

@den-run-ai
Copy link
Contributor

@filmor can you add tests compatible with both python 2 and 3 that use cPickle:

try:
   import cPickle as pickle
except:
   import pickle

@filmor
Copy link
Member Author

filmor commented Nov 9, 2016

@denfromufa Done. Please check :)

@den-run-ai
Copy link
Contributor

So is this a "feature" or "bug fix"? If latter, then let's merge this for
v2.2.0! If not, can we wait until I release binaries for v2.2.0?

Have you ran the embedding tests to make sure that exceptions do not break
on the other side of the bridge?

On Wed, Nov 9, 2016 at 4:22 AM, Benedikt Reinartz notifications@github.com
wrote:

@denfromufa https://github.com/denfromufa Done. Please check :)


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#286 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AHgZ5YJv7_fge093W5Qyqjpt9lSXkpmnks5q8Z72gaJpZM4KsPc9
.

@filmor
Copy link
Member Author

filmor commented Nov 9, 2016

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).

@den-run-ai
Copy link
Contributor

Ok, please let me know if you have any luck running embedding tests and
then we can merge this for v2.2.0.

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
wrote:

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).


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#286 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AHgZ5eVtKokXm3Jlt06_TqaF3qQ8Ei7xks5q8eS6gaJpZM4KsPc9
.

@filmor
Copy link
Member Author

filmor commented Nov 9, 2016

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.

@den-run-ai den-run-ai merged commit db9478f into pythonnet:master Nov 10, 2016
@filmor filmor deleted the fix-exception-pickling branch November 25, 2016 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants