| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| Expand Up | @@ -396,15 +396,18 @@ static const char * const sys_files[] = { | |
| void | ||
| PyImport_Cleanup(void) | ||
| { | ||
| assert(!PyErr_Occurred()); | ||
|
Comment thread
Copy link
Copy Markdown
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityI'm wondering if it is worth to call PyErr_WriteUnraisable() here. This will expose some information about the exception raised outside of this function.
Sorry, something went wrong.
All reactions
Copy link
Copy Markdown
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityThe contract of this function is that the caller must not hold an exception. If the assertion fails, I would prefer to fix the caller rather than working around the bug using PyErr_WriteUnraisable(). The point is not only to catch bugs in the current code, but also ease debug when the callers code will be modified. Python shutdown process is very fragile and error prone.
Sorry, something went wrong.
All reactions
|
||
|
|
||
| Py_ssize_t pos; | ||
| PyObject *key, *value, *dict; | ||
| PyInterpreterState *interp = PyThreadState_GET()->interp; | ||
| PyObject *modules = PyImport_GetModuleDict(); | ||
| PyObject *weaklist = NULL; | ||
| const char * const *p; | ||
|
|
||
| if (modules == NULL) | ||
| PyObject *modules = interp->modules; | ||
|
Comment thread
Copy link
Copy Markdown
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityI'd rather avoid this, if possible. Under what conditions does the use of PyImport_GetModuleDict() cause a fatal error?
Sorry, something went wrong.
All reactions
Copy link
Copy Markdown
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityIf you call PyImport_Cleanup() twice. In Python 3.6, calling PyImport_Cleanup() a second time does nothing: that's the purpose of the test on the next line. PyImport_Cleanup() is part of the public API, even if it's documented as "please don't call this function" ;-) I would prefer to restore Python 3.6 behaviour.
Sorry, something went wrong.
serhiy-storchaka reacted with thumbs up emoji
All reactions
Copy link
Copy Markdown
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityAh, thanks for clarifying. LGTM then. I still plan on landing #3606 at some point, but at that point I'll have to deal with recent interp->modules usage anyway. :)
Sorry, something went wrong.
All reactions
|
||
| if (modules == NULL) { | ||
| return; /* Already done */ | ||
| } | ||
|
|
||
| /* Delete some special variables first. These are common | ||
| places where user values hide and people complain when their | ||
| Expand Down Expand Up | @@ -437,6 +440,7 @@ PyImport_Cleanup(void) | |
| PyErr_WriteUnraisable(NULL); | ||
| } | ||
| } | ||
| assert(!PyErr_Occurred()); | ||
|
|
||
| /* We prepare a list which will receive (name, weakref) tuples of | ||
| modules when they are removed from sys.modules. The name is used | ||
| Expand Down Expand Up | @@ -477,6 +481,7 @@ PyImport_Cleanup(void) | |
| if (PyDict_CheckExact(modules)) { | ||
| pos = 0; | ||
| while (PyDict_Next(modules, &pos, &key, &value)) { | ||
| assert(!PyErr_Occurred()); | ||
|
Comment thread
Copy link
Copy Markdown
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityNeither PyDict_Next() nor any other calls before can raise an exception.
Sorry, something went wrong.
All reactions
Copy link
Copy Markdown
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityThis is the theory. In practice, something called before might leak an exception. It's to catch bugs "just in case". The assertion is cheap. Debugging an "exception leak" can be very painful.
Sorry, something went wrong.
All reactions
Copy link
Copy Markdown
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityThere is no code between this line and previous error check that can raise an exception and don't silence it. I have checked this.
Sorry, something went wrong.
All reactions
|
||
| CLEAR_MODULE(key, value); | ||
| } | ||
| } | ||
| Expand All | @@ -492,6 +497,7 @@ PyImport_Cleanup(void) | |
| PyErr_WriteUnraisable(NULL); | ||
| continue; | ||
| } | ||
| assert(!PyErr_Occurred()); | ||
|
Comment thread
Copy link
Copy Markdown
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityPyObject_GetIter(), PyIter_Next() and PyObject_GetItem() can raise and exception if corresponding slots are implemented incorrectly in extensions. assert looks good here.
Sorry, something went wrong.
All reactions
|
||
| CLEAR_MODULE(key, value); | ||
| Py_DECREF(value); | ||
| Py_DECREF(key); | ||
| Expand All | @@ -502,6 +508,7 @@ PyImport_Cleanup(void) | |
| Py_DECREF(iterator); | ||
| } | ||
| } | ||
| assert(!PyErr_Occurred()); | ||
|
Comment thread
Copy link
Copy Markdown
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityThis never fails.
Sorry, something went wrong.
All reactions
|
||
|
|
||
| /* Clear the modules dict. */ | ||
| if (PyDict_CheckExact(modules)) { | ||
| Expand All | @@ -524,6 +531,8 @@ PyImport_Cleanup(void) | |
| PyErr_Clear(); | ||
| } | ||
| Py_XDECREF(dict); | ||
| assert(!PyErr_Occurred()); | ||
|
Comment thread
Copy link
Copy Markdown
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityThis never fails.
Sorry, something went wrong.
All reactions
|
||
|
|
||
| /* Clear module dict copies stored in the interpreter state */ | ||
| _PyState_ClearModules(); | ||
| /* Collect references */ | ||
| Expand All | @@ -543,6 +552,7 @@ PyImport_Cleanup(void) | |
| references left to it), we need to delete the "builtins" | ||
| module last. Likewise, we don't delete sys until the very | ||
| end because it is implicitly referenced (e.g. by print). */ | ||
| assert(!PyErr_Occurred()); | ||
|
Comment thread
Copy link
Copy Markdown
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityThis never fails.
Sorry, something went wrong.
All reactions
|
||
| if (weaklist != NULL) { | ||
| Py_ssize_t i, n; | ||
| n = PyList_GET_SIZE(weaklist); | ||
| Expand All | @@ -559,6 +569,7 @@ PyImport_Cleanup(void) | |
| Py_INCREF(mod); | ||
| if (Py_VerboseFlag && PyUnicode_Check(name)) | ||
| PySys_FormatStderr("# cleanup[3] wiping %U\n", name); | ||
| assert(!PyErr_Occurred()); | ||
|
Comment thread
Copy link
Copy Markdown
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityThis never fails.
Sorry, something went wrong.
All reactions
|
||
| _PyModule_Clear(mod); | ||
| Py_DECREF(mod); | ||
| } | ||
| Expand Down | ||
| Back | FazBrowse Home | New Git URL |
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.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityYesterday I wrote a patch that improves errors handling in the garbage collector. bpo-33612 is the result of testing it. And I have added similar asserts in _PyGC_CollectNoFail() (but the patch is not completed yet, maybe I'll move them).
At least move the second assert inside the "else" block.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.