bpo-41336: Fix the error handling in zoneinfo_new_instance() #21546
Conversation
|
I believe this is a "skip news" PR. |
ZackerySpytz
force-pushed
the
bpo-41336-zoneinfo-exception-handling
branch
from
054080e to
441c715
Compare
Do not call PyObject_CallMethod() with a live exception (like KeyboardInterrupt).
ZackerySpytz
force-pushed
the
bpo-41336-zoneinfo-exception-handling
branch
from
441c715 to
dd39080
Compare
serhiy-storchaka
added
needs backport to 3.9
|
Thanks @ZackerySpytz for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9. |
…H-21546) Do not call PyObject_CallMethod() with a live exception (like KeyboardInterrupt). (cherry picked from commit eca2549) Co-authored-by: Zackery Spytz <[email protected]>
|
GH-21563 is a backport of this pull request to the 3.9 branch. |
Do not call PyObject_CallMethod() with a live exception (like KeyboardInterrupt). (cherry picked from commit eca2549) Co-authored-by: Zackery Spytz <[email protected]>
| @@ -224,8 +224,14 @@ zoneinfo_new_instance(PyTypeObject *type, PyObject *key) | |||
| self = NULL; | |||
| cleanup: | |||
| if (file_obj != NULL) { | |||
| PyObject *exc, *val, *tb; | |||
| PyErr_Fetch(&exc, &val, &tb); | |||
There was a problem hiding this comment.
What is the purpose of this?
Is the issue that there might be a live exception and that file_obj.close() may itself raise an exception, so we want to save the original one and chain it to the new one?
| Py_DECREF(tmp); | ||
| _PyErr_ChainExceptions(exc, val, tb); | ||
| if (tmp == NULL) { | ||
| Py_CLEAR(self); |
There was a problem hiding this comment.
Hm... this feels a little weird but I suppose it's acceptable.
At this point we've actually already succeeded in reading and constructing a ZoneInfo in self; if tmp is NULL that indicates that we've failed to close a file that we've opened for some reason, but we could still return self. In what situation would it matter that we failed to close the file handle? I would expect that failure to close the file handle would mean that it's already closed or something has happened to the file descriptor at the OS level — some sort of conflict around ownership of the resource that we could easily cede to whatever other process has taken ownership of it.
Is the idea here that the Ctrl + C here is manifesting as an error in the method call and will only bubble up the stack if we affirmatively raise it here? If so, might it make sense to clear anything derived from Exception and bubble up only errors here derived from BaseException but not Exception?
…H-21546) Do not call PyObject_CallMethod() with a live exception (like KeyboardInterrupt).
…H-21546) Do not call PyObject_CallMethod() with a live exception (like KeyboardInterrupt).
…H-21546) Do not call PyObject_CallMethod() with a live exception (like KeyboardInterrupt).
…H-21546) Do not call PyObject_CallMethod() with a live exception (like KeyboardInterrupt).
Do not call PyObject_CallMethod() with a live exception (like
KeyboardInterrupt).
https://bugs.python.org/issue41336