FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-98778: Update HTTPError to initialize properly even if fp is None by corona10 · Pull Request #99966 · python/cpython · GitHub

/ cpython Public

gh-98778: Update HTTPError to initialize properly even if fp is None - #99966

Merged
corona10 merged 3 commits into
python:mainfrom
corona10:gh-98778
Dec 8, 2022
ConversationCommits (3)ChecksFiles changed
Merged

gh-98778: Update HTTPError to initialize properly even if fp is None#99966
corona10 merged 3 commits into
python:mainfrom
corona10:gh-98778

Conversation

corona10 commented Dec 3, 2022 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

corona10 added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Dec 3, 2022
bedevere-bot mentioned this pull request Dec 3, 2022
Closed

corona10 commented Dec 3, 2022

Copy link
Copy Markdown
Member Author

Since the _TemporaryFileCloser is used for internal usage, this change will not impact to 3rd party libraries.

corona10 commented Dec 3, 2022

Copy link
Copy Markdown
Member Author 
(.oss) ➜  cpython git:(gh-98778) ✗ ./python.exe -m test test_urllib2 -R 3:3
Raised RLIMIT_NOFILE: 256 -> 1024
0:00:00 load avg: 1.44 Run tests sequentially
0:00:00 load avg: 1.44 [1/1] test_urllib2
beginning 6 repetitions
123456
......

== Tests result: SUCCESS ==

1 test OK.

Total duration: 1.5 sec
Tests result: SUCCESS

netlify Bot commented Dec 7, 2022 •
edited
Loading

Copy link
Copy Markdown

✅ Deploy Preview for python-cpython-preview canceled.

Name Link
🔨 Latest commit c3a75a1
🔍 Latest deploy log https://app.netlify.com/sites/python-cpython-preview/deploys/63913fd79fffe30008780f7a

ambv requested changes Dec 7, 2022
View reviewed changes

ambv left a comment

Copy link
Copy Markdown
Contributor

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 Quality

You made changes to _TemporaryFileCloser but urllib.response.addinfourl here is a subclass of _TemporaryFileWrapper. Now after your changes the .close() and .cleanup() operations work but the wrapper is still in an inconsistent state. It cannot be used as a context manager, and cannot be iterated. The wrapper proxies attribute access to the file handle so asking things like exc.mode, exc.seekable(), etc. will all fail.

The cleanest solution would be to change HTTPError to not subclass this thing at all... but that ship has sailed a long time ago.

Your change weakens _TemporaryFileCloser by allowing it to silently accept None in lieu of a file handle. This makes tempfile worse, I'm not convinced this is a good compromise.

Instead, since HTTPError is already a hack (and there was literally a comment about why it's hacky where the bug was created), I would just replace the hack with a different one: set fp to io.StringIO() if the passed fp is None. It's still not perfect but I feel that would be more robust.

bedevere-bot commented Dec 7, 2022

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

corona10 commented Dec 7, 2022

Copy link
Copy Markdown
Member Author

Instead, since HTTPError is already a hack (and there was literally a comment about why it's hacky where the bug was created), I would just replace the hack with a different one: set fp to io.StringIO() if the passed fp is None. It's still not perfect but I feel that would be more robust.

+1

corona10 commented Dec 7, 2022

Copy link
Copy Markdown
Member Author

@ambv

Thank you for the review and for pointing out what I missed :)

I have made the requested changes; please review again

bedevere-bot commented Dec 7, 2022

Copy link
Copy Markdown

Thanks for making the requested changes!

@ambv: please review the changes made to this pull request.

bedevere-bot requested a review from ambv December 7, 2022 15:28
ambv approved these changes Dec 7, 2022
View reviewed changes
Comment thread Lib/urllib/error.py Outdated
Update Lib/urllib/error.py
c3a75a1 
Co-authored-by: Łukasz Langa <lukasz@langa.pl>
corona10 merged commit dc8a868 into python:main Dec 8, 2022

miss-islington commented Dec 8, 2022

Copy link
Copy Markdown
Contributor

Thanks @corona10 for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

corona10 deleted the gh-98778 branch December 8, 2022 02:20

bedevere-bot commented Dec 8, 2022

Copy link
Copy Markdown

GH-100096 is a backport of this pull request to the 3.11 branch.

bedevere-bot removed the needs backport to 3.11 only security fixes label Dec 8, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 8, 2022
pythongh-98778: Update HTTPError to initialize properly even if fp is…
c699a00 
… None (pythongh-99966)

(cherry picked from commit dc8a868)

Co-authored-by: Dong-hee Na <donghee.na@python.org>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 8, 2022
pythongh-98778: Update HTTPError to initialize properly even if fp is…
b39b498 
… None (pythongh-99966)

(cherry picked from commit dc8a868)

Co-authored-by: Dong-hee Na <donghee.na@python.org>

bedevere-bot commented Dec 8, 2022

Copy link
Copy Markdown

GH-100097 is a backport of this pull request to the 3.10 branch.

bedevere-bot removed the needs backport to 3.10 only security fixes label Dec 8, 2022
miss-islington added a commit that referenced this pull request Dec 8, 2022
gh-98778: Update HTTPError to initialize properly even if fp is None (g…
846898e 
…h-99966)

(cherry picked from commit dc8a868)

Co-authored-by: Dong-hee Na <donghee.na@python.org>
miss-islington added a commit that referenced this pull request Dec 8, 2022
gh-98778: Update HTTPError to initialize properly even if fp is None (g…
85be258 
…h-99966)

(cherry picked from commit dc8a868)

Co-authored-by: Dong-hee Na <donghee.na@python.org>
gaborbernat reviewed Feb 15, 2023
View reviewed changes
Comment thread Lib/urllib/error.py
if fp is not None:
self.__super_init(fp, hdrs, url, code)
if fp is None:
fp = io.StringIO()

gaborbernat Feb 15, 2023

Copy link
Copy Markdown
Contributor

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 Quality

Shouldn't this be BytesIO?

from urllib.request import urlopen
from urllib.error import HTTPError
try:
    urlopen('http://asadsad.sd')
except HTTPError as exception:
    content = exception.fp.read()
    print(type(content))
<class 'bytes'>

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ambv ambv approved these changes
serhiy-storchaka Awaiting requested review from serhiy-storchaka
orsenthil Awaiting requested review from orsenthil
jaraco Awaiting requested review from jaraco
+1 more reviewer
gaborbernat gaborbernat left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL