Skip to content
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

bpo-31271: fix an assertion failure in io.TextIOWrapper.write #3201

Merged
merged 3 commits into from
Aug 25, 2017

Conversation

orenmn
Copy link
Contributor

@orenmn orenmn commented Aug 24, 2017

  • in textio.c: add a check whether encoder's encode() returned a bytes object.
  • in test_io.py: add a test to verify the assertion doesn't fail anymore.

https://bugs.python.org/issue31271

return BadEncoder()
quopri = codecs.lookup("quopri")
with support.swap_attr(quopri, '_is_text_encoding', True), \
support.swap_attr(quopri, 'incrementalencoder', _get_bad_encoder):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is monkey-patching incrementalencoder needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I understood your question.

  • if we remove it, then the assertion won't fail (even without the issue's patch).
  • if we just overwrite quopri.incrementalencoder, without restoring it, the assertion would fail (without the issue's patch), but I am not sure about the effects of not restoring it (on Python with the issue's patch), when Python keeps running.
  • can we reproduce the assertion failure without changing quopri.incrementalencoder? I don't know. I haven't looked for any other way, but I can do that, if you think it's worth it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if use an existing encoding that produces non-bytes? E.g. "rot13"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed. I would update the patch.

@orenmn
Copy link
Contributor Author

orenmn commented Aug 25, 2017

is a NEWS.d entry necessary?

@serhiy-storchaka
Copy link
Member

This would not hurt.

Copy link
Contributor

@ncoghlan ncoghlan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix looks good to me as well, but the automated check is correct that this does need a NEWS entry.

…ilar error when the decoder doesn't return a str.
@serhiy-storchaka serhiy-storchaka merged commit a5b4ea1 into python:master Aug 25, 2017
orenmn added a commit to orenmn/cpython that referenced this pull request Aug 26, 2017
orenmn added a commit to orenmn/cpython that referenced this pull request Aug 26, 2017
@bedevere-bot
Copy link

GH-3209 is a backport of this pull request to the 3.6 branch.

@miss-islington
Copy link
Contributor

Thanks @orenmn for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 13, 2017
@bedevere-bot
Copy link

GH-3548 is a backport of this pull request to the 2.7 branch.

@miss-islington
Copy link
Contributor

Thanks @orenmn for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 16, 2017
@bedevere-bot
Copy link

bedevere-bot commented Sep 16, 2017

GH-3951 is a backport of this pull request to the 2.7 branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants