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

Jump to bottom

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

Closed
vxgmichel opened this issue Oct 27, 2022 · 3 comments
Closed

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

vxgmichel opened this issue Oct 27, 2022 · 3 comments
Labels
type-bug An unexpected behavior, bug, or error

Comments

@vxgmichel
Copy link
Contributor

vxgmichel commented Oct 27, 2022 •
edited by bedevere-bot

Bug report

The exception urllib.error.HTTPError(..., fp=None) raises a KeyError instead of an AttributeError when accessing an attribute that does not exist.

>>> from urllib.error import HTTPError
>>> x = HTTPError("url", 405, "METHOD NOT ALLOWED", None, None)
>>> assert getattr(x, "__notes__", ())  == ()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/vinmic/repos/cpython/Lib/tempfile.py", line 477, in __getattr__
    file = self.__dict__['file']
           ~~~~~~~~~~~~~^^^^^^^^
KeyError: 'file'

Your environment

Python 3.12.0a1+ (heads/main:bded5edd9a, Oct 27 2022, 19:23:58) [GCC 11.3.0] on linux

This bug should be reproducible for all python3 versions on all systems.

Context

I found this error while running a code similar to this:

from logging import getLogger
from urllib.error import HTTPError
try:
    raise HTTPError("url", 405, "METHOD NOT ALLOWED", None, None)
except Exception:
    getLogger().exception("Ooops")

Instead of having the exception logged, I ended up with the following trace:

--- Logging error ---
Traceback (most recent call last):
  File "/home/vinmic/repos/cpython/../test_trio/test_trio.py", line 6, in <module>
    raise HTTPError("url", 405, "METHOD NOT ALLOWED", None, None)
urllib.error.HTTPError: HTTP Error 405: METHOD NOT ALLOWED

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/vinmic/repos/cpython/Lib/logging/__init__.py", line 1160, in emit
    msg = self.format(record)
          ^^^^^^^^^^^^^^^^^^^
  File "/home/vinmic/repos/cpython/Lib/logging/__init__.py", line 999, in format
    return fmt.format(record)
           ^^^^^^^^^^^^^^^^^^
  File "/home/vinmic/repos/cpython/Lib/logging/__init__.py", line 711, in format
    record.exc_text = self.formatException(record.exc_info)
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/vinmic/repos/cpython/Lib/logging/__init__.py", line 661, in formatException
    traceback.print_exception(ei[0], ei[1], tb, None, sio)
  File "/home/vinmic/repos/cpython/Lib/traceback.py", line 124, in print_exception
    te = TracebackException(type(value), value, tb, limit=limit, compact=True)
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/vinmic/repos/cpython/Lib/traceback.py", line 697, in __init__
    self.__notes__ = getattr(exc_value, '__notes__', None)
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/vinmic/repos/cpython/Lib/tempfile.py", line 477, in __getattr__
    file = self.__dict__['file']
           ~~~~~~~~~~~~~^^^^^^^^
KeyError: 'file'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/vinmic/repos/cpython/../test_trio/test_trio.py", line 8, in <module>
    getLogger().exception("Ooops")
  File "/home/vinmic/repos/cpython/Lib/logging/__init__.py", line 1574, in exception
    self.error(msg, *args, exc_info=exc_info, **kwargs)
  File "/home/vinmic/repos/cpython/Lib/logging/__init__.py", line 1568, in error
    self._log(ERROR, msg, args, **kwargs)
  File "/home/vinmic/repos/cpython/Lib/logging/__init__.py", line 1684, in _log
    self.handle(record)
  File "/home/vinmic/repos/cpython/Lib/logging/__init__.py", line 1700, in handle
    self.callHandlers(record)
  File "/home/vinmic/repos/cpython/Lib/logging/__init__.py", line 1770, in callHandlers
    lastResort.handle(record)
  File "/home/vinmic/repos/cpython/Lib/logging/__init__.py", line 1028, in handle
    self.emit(record)
  File "/home/vinmic/repos/cpython/Lib/logging/__init__.py", line 1168, in emit
    self.handleError(record)
  File "/home/vinmic/repos/cpython/Lib/logging/__init__.py", line 1082, in handleError
    traceback.print_exception(t, v, tb, None, sys.stderr)
  File "/home/vinmic/repos/cpython/Lib/traceback.py", line 124, in print_exception
    te = TracebackException(type(value), value, tb, limit=limit, compact=True)
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/vinmic/repos/cpython/Lib/traceback.py", line 763, in __init__
    context = TracebackException(
              ^^^^^^^^^^^^^^^^^^^
  File "/home/vinmic/repos/cpython/Lib/traceback.py", line 697, in __init__
    self.__notes__ = getattr(exc_value, '__notes__', None)
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/vinmic/repos/cpython/Lib/tempfile.py", line 477, in __getattr__
    file = self.__dict__['file']
           ~~~~~~~~~~~~~^^^^^^^^
KeyError: 'file'

Note however that the exception is logged properly on python 3.9 (without the exceptiongroup module imported). So maybe the following patch should be applied on top of HTTPError being fixed in order to make tracebacks more robust:

diff --git a/Lib/traceback.py b/Lib/traceback.py
index 6270100348..a9c15d59be 100644
--- a/Lib/traceback.py
+++ b/Lib/traceback.py
@@ -694,7 +694,10 @@ def __init__(self, exc_type, exc_value, exc_traceback, *, limit=None,
         # Capture now to permit freeing resources: only complication is in the
         # unofficial API _format_final_exc_line
         self._str = _safe_string(exc_value, 'exception')
-        self.__notes__ = getattr(exc_value, '__notes__', None)
+        try:
+            self.__notes__ = getattr(exc_value, '__notes__', None)
+        except Exception:
+            self.__notes__ = None
 
         if exc_type and issubclass(exc_type, SyntaxError):
             # Handle SyntaxError's specially

Linked PRs
@vxgmichel vxgmichel added the type-bug An unexpected behavior, bug, or error label Oct 27, 2022
@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Oct 28, 2022

The problem is that HTTPError is a subclass of tempfile._TemporaryFileWrapper, which is not properly initialized if fp is None.

cpython/Lib/urllib/error.py

Lines 49 to 50 in 3e07f82

if fp is not None:
self.__super_init(fp, hdrs, url, code)

cc @orsenthil

@brondsem brondsem mentioned this issue Nov 11, 2022
Closed
@bedevere-bot bedevere-bot mentioned this issue Dec 3, 2022
Merged
corona10 added a commit to corona10/cpython that referenced this issue Dec 3, 2022
@corona10
pythongh-98778: Update HTTPError to initialize properly even if fp is…
003dbd9 
… None
corona10 added a commit to corona10/cpython that referenced this issue Dec 7, 2022
@corona10
pythongh-98778: Update HTTPError to initialize properly even if fp is…
448231f 
… None
@bedevere-bot bedevere-bot mentioned this issue Dec 8, 2022
Merged
corona10 added a commit that referenced this issue Dec 8, 2022
@corona10
gh-98778: Update HTTPError to initialize properly even if fp is None (g…
dc8a868 
…h-99966)
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Dec 8, 2022
@corona10 @miss-islington
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>
@bedevere-bot bedevere-bot mentioned this issue Dec 8, 2022
Merged
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Dec 8, 2022
@corona10 @miss-islington
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>
@corona10 corona10 closed this as completed Dec 8, 2022
miss-islington added a commit that referenced this issue Dec 8, 2022
@miss-islington @corona10
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 issue Dec 8, 2022
@miss-islington @corona10
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>
@vxgmichel
Copy link
Contributor Author

vxgmichel commented Dec 8, 2022

Great! Thanks for the fix :)

Do you think it would be interesting to make traceback more robust and protect it against those kinds of buggy exceptions? I suggested the following patch in my original comment:

diff --git a/Lib/traceback.py b/Lib/traceback.py
index 6270100348..a9c15d59be 100644
--- a/Lib/traceback.py
+++ b/Lib/traceback.py
@@ -694,7 +694,10 @@ def __init__(self, exc_type, exc_value, exc_traceback, *, limit=None,
         # Capture now to permit freeing resources: only complication is in the
         # unofficial API _format_final_exc_line
         self._str = _safe_string(exc_value, 'exception')
-        self.__notes__ = getattr(exc_value, '__notes__', None)
+        try:
+            self.__notes__ = getattr(exc_value, '__notes__', None)
+        except Exception:
+            self.__notes__ = None
 
         if exc_type and issubclass(exc_type, SyntaxError):
             # Handle SyntaxError's specially

@corona10
Copy link
Member

corona10 commented Dec 8, 2022

@vxgmichel
I think that you can open a new issue if you want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@serhiy-storchaka @corona10 @vxgmichel