| msg249628 - (view) |
Author: STINNER Victor (vstinner) *  |
Date: 2015-09-03 10:49 |
| The namereplace error handler introduced in Python 3.5 doesn't handle correctly PyCapsule_Import() failure. If the function raises an exception, the PyCodec_NameReplaceErrors() function must return NULL. I see that the code correctly handle the case where PyCodec_NameReplaceErrors() failed, but it doesn't clear the exception. Attached patch changes PyCodec_NameReplaceErrors() to return immediatly NULL if PyCodec_NameReplaceErrors() failed. Or should we log the exception (PyErr_WriteUnraisable) and then clear it? PyErr_WriteUnraisable is usually used in corner case when it's impossible to report bugs to the function caller. |
|
|
| msg249629 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2015-09-03 10:50 |
| Note: I found the bug when running test_codecs using failmalloc, a library to inject MemoryError. |
|
|
| msg249637 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2015-09-03 13:35 |
| The purpose was to make the dependence of unicodedata optional. Here is a patch that adds missing lines to clear import error. But your approach looks reasonable too and the patch is correct. I have no strong preferences, but now I think that probably your approach is more preferable because it makes the code simpler and errors more explicit. Please commit your patch Victor if you don't see new reasons for original approach. |
|
|
| msg249638 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2015-09-03 13:40 |
| > The purpose was to make the dependence of unicodedata optional. In which case the unicodedata module is missing? It's always part of CPython no? > (...) but now I think that probably your approach is more preferable because it makes the code simpler and errors more explicit. Yeah, it's not a good practice to *hide* errors. At least, your patch must log the error. |
|
|
| msg249642 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2015-09-03 14:15 |
| > In which case the unicodedata module is missing? It's always part of CPython > no? It was optional in Python 2 (at least in Unicode-disabled build). Perhaps it can be exclude from custom build in Python 3 too. > Yeah, it's not a good practice to *hide* errors. At least, your patch must > log the error. Then please commit your patch. We can reenable optional dependency later if it will be needed for embedded systems. |
|
|
| msg249643 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2015-09-03 14:18 |
| > It was optional in Python 2 (at least in Unicode-disabled build). Yeah, even if I don't think that anyone is really using python without unicode in the wild. I guess that too many libraries don't work without unicode. Well, anyway this issue is for Python 3.6 which has always unicode support :-) > Perhaps it can be exclude from custom build in Python 3 too. Right, some patches were proposed to disable some features of Python to get a smaller Python core/stdlib, but some developers were opposed to this idea. It's not supported officially by Python, so I don't think that we should polute the code for an hypothetic use case. If you use a custom build, you must be prepared to some corner case bugs. If you modify the build, you are able to fix such simple issue. |
|
|
| msg249644 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2015-09-03 14:20 |
| New changeset ac1995b01028 by Victor Stinner in branch '3.5': Issue #24993: Handle import error in namereplace error handler https://hg.python.org/cpython/rev/ac1995b01028 |
|
|
| msg249650 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2015-09-03 14:40 |
| Thanks for your review and your cool error handler. |
|
|