-
Notifications
You must be signed in to change notification settings - Fork 27
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
Avoid raising a SystemError when clearing slots if setstate() failed. #62
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, except for the exc=NotImplementedError
bit noted.
persistent/tests/test_persistence.py
Outdated
@@ -68,7 +71,7 @@ def register(self, obj): | |||
jar._cache = self._makeCache(jar) | |||
return jar | |||
|
|||
def _makeBrokenJar(self): | |||
def _makeBrokenJar(self, exc=NotImplementedError): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This extra indirection seems like YAGNI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I was imagining that NotImplementedError
might be caught and handled specially somewhere so I wanted to be sure to have something that would definitely propagate. But that is hypothetical, I admit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
PR #52 introduced a code path to `ghostify` that calls PyErr_Clear() with the intent to avoid propagating AttributeErrors for slots. However, if there is an error (like a POSKeyError) raised by jar.setstate(), then `unghostify` will call ghostify with an error pending. If the object had slots that weren't set and the AttributeError was cleared, so was the pending error from setstate. So when `ghostify` returned NULL that got propagated up to the interpreter which finds no exception and so raises `SystemError: error return without exception set`. This commit makes `unghostify` save and restore the exception state around the call to PyErr_Clear.
b7846d4
to
e8b9c8e
Compare
@tseaver Could we get a merge and release please? |
Thank you! |
The fix was done upstream at: zopefoundation/persistent#62
PR #52 introduced a code path to
ghostify
that calls PyErr_Clear() with the intent to avoid propagatingAttributeErrors
for slots.However, if there is an error (like a POSKeyError) raised by jar.setstate(), then
unghostify
will callghostify
with an error pending. If the object had slots that weren't set and theAttributeError
was cleared, so was the pending error from setstate. So whenghostify
returned NULL that got propagated up to the interpreter which finds no exception and so raisesSystemError: error return without exception set
.This commit makes
unghostify
save and restore the exception state around the call toPyErr_Clear
so the real exception is raised, rather than the baffling and unhelpful SystemError.