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

exceptions: ensure ProcessExecutionError can be pickled #586

Merged
merged 3 commits into from
Feb 3, 2022

Conversation

koreno
Copy link
Collaborator

@koreno koreno commented Feb 2, 2022

Something in recent refactors broke this capability, which we are using in our infrastructure, so I've fixed and cemented it with a UT.

@koreno koreno requested a review from henryiii February 2, 2022 23:10
@henryiii
Copy link
Collaborator

henryiii commented Feb 3, 2022

EnvironmentError isn't a real error anymore in Python 3, it's just an OSError. OSErrors return subclasses based on what the OS return code in the first argument is. I think we need to refactor this a bit. Specifically, the first value is the return code, and for us it's the second, and OSError's new is where the subclass selection is happening - this sounds very incorrect to me! For now, this is fine to fix the immediate problem.

@henryiii
Copy link
Collaborator

henryiii commented Feb 3, 2022

Maybe it is implemented in __init__, since this seems to be stable (after the patch), or maybe it checks the cls before doing the subclass (though EnvironementError(3, "msg") returns ProcessLookupError(3, 'msg'), etc). The exceptions are mostly implemented in C, so hard to quickly check.

@coveralls
Copy link

coveralls commented Feb 3, 2022

Coverage Status

Coverage remained the same at 83.608% when pulling a7c656e on fix-exception-pickling into 7bba255 on master.

@henryiii henryiii merged commit ba4b1a0 into master Feb 3, 2022
@henryiii
Copy link
Collaborator

henryiii commented Feb 3, 2022

Thanks!!!

@henryiii henryiii deleted the fix-exception-pickling branch February 3, 2022 16:01
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.

3 participants