This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: The C function PyCodec_NameReplaceErrors doesn't handle PyCapsule_Import() failure
Type: behavior Stage: commit review
Components: Versions: Python 3.6, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: vstinner Nosy List: python-dev, serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2015-09-03 10:49 by vstinner, last changed 2022-04-11 14:58 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
namereplace_errors.patch vstinner, 2015-09-03 10:49 review
namereplace_ignore_unicodedata_import_error.patch serhiy.storchaka, 2015-09-03 13:35 review
Messages (8)
msg249628 - (view) Author: STINNER Victor (vstinner) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) (Python triager) 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) * (Python committer) Date: 2015-09-03 14:40
Thanks for your review and your cool error handler.
History
Date User Action Args
2022-04-11 14:58:20adminsetgithub: 69181
2015-09-03 14:40:27vstinnersetmessages: + msg249650
2015-09-03 14:21:12vstinnersetstatus: open -> closed
resolution: fixed
2015-09-03 14:20:53python-devsetnosy: + python-dev
messages: + msg249644
2015-09-03 14:18:35vstinnersetmessages: + msg249643
2015-09-03 14:15:08serhiy.storchakasetmessages: + msg249642
2015-09-03 13:40:13vstinnersetmessages: + msg249638
2015-09-03 13:35:58serhiy.storchakasetfiles: + namereplace_ignore_unicodedata_import_error.patch
messages: + msg249637

assignee: vstinner
type: behavior
stage: commit review
2015-09-03 10:50:18vstinnersetmessages: + msg249629
2015-09-03 10:49:48vstinnercreate