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

[corona10]

• Issue: urllib.error.HTTPError(..., fp=None) raises a KeyError instead of an AttributeError on attribute access #98778

[corona10]

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

[corona10]

(.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]

✅ 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]

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]

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

[corona10]

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]

@ambv

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

I have made the requested changes; please review again

[bedevere-bot]

Thanks for making the requested changes!

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

[miss-islington]

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

[bedevere-bot]

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

[bedevere-bot]

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

[gaborbernat]

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'>