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

urllib.request.urlopen leaks exceptions from socket and httplib.client #57945

Open
jmoy mannequin opened this issue Jan 8, 2012 · 7 comments
Open

urllib.request.urlopen leaks exceptions from socket and httplib.client #57945

jmoy mannequin opened this issue Jan 8, 2012 · 7 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@jmoy
Copy link
Mannequin

jmoy mannequin commented Jan 8, 2012

BPO 13736
Nosy @orsenthil, @pitrou, @ezio-melotti, @merwok, @vadmium, @jmoy
Files
  • test_urllib_except.py: Test to show bug.
  • urllib_exception_leak.patch: Patch to fix issue urllib.request.urlopen leaks exceptions from socket and httplib.client #57945
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/orsenthil'
    closed_at = None
    created_at = <Date 2012-01-08.06:22:19.699>
    labels = ['type-bug', 'library']
    title = 'urllib.request.urlopen leaks exceptions from socket and httplib.client'
    updated_at = <Date 2015-11-16.22:14:15.896>
    user = 'https://github.com/jmoy'

    bugs.python.org fields:

    activity = <Date 2015-11-16.22:14:15.896>
    actor = 'martin.panter'
    assignee = 'orsenthil'
    closed = False
    closed_date = None
    closer = None
    components = ['Library (Lib)']
    creation = <Date 2012-01-08.06:22:19.699>
    creator = 'jmoy'
    dependencies = []
    files = ['24172', '24173']
    hgrepos = []
    issue_num = 13736
    keywords = ['patch']
    message_count = 7.0
    messages = ['150849', '150850', '151690', '151697', '151698', '152881', '254761']
    nosy_count = 6.0
    nosy_names = ['orsenthil', 'pitrou', 'ezio.melotti', 'eric.araujo', 'martin.panter', 'jmoy']
    pr_nums = []
    priority = 'normal'
    resolution = None
    stage = None
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue13736'
    versions = ['Python 3.3']

    @jmoy
    Copy link
    Mannequin Author

    jmoy mannequin commented Jan 8, 2012

    The documentation for urlopen says that it raises URLError on error.
    But there exist error conditions such as a socket timeout or a bad HTTP status line under which the exception from the underlying library leaks through urllib, thus breaking the promise in the documentation.

    I am attaching a test program that demonstrates this bug.

    @jmoy jmoy mannequin added the stdlib Python modules in the Lib dir label Jan 8, 2012
    @jmoy
    Copy link
    Mannequin Author

    jmoy mannequin commented Jan 8, 2012

    A patch to fix this issue. Catches exceptions from underlying libraries and reraises them as URLError.

    I put the class name of the underlying exception in the reason to make it more descriptive.

    @jmoy jmoy mannequin added the type-bug An unexpected behavior, bug, or error label Jan 8, 2012
    @orsenthil orsenthil self-assigned this Jan 20, 2012
    @pitrou
    Copy link
    Member

    pitrou commented Jan 20, 2012

    I think the general pattern of wrapping exceptions in other exceptions is rather unfortunate. It makes it harder to examine the original problem (for example the errno attribute) for no actual gain.

    @jmoy
    Copy link
    Mannequin Author

    jmoy mannequin commented Jan 20, 2012

    Antoine, could "raise ... from" be used here? Perhaps also using new subclasses of URLError to allow the exceptions to be caught with finer granularity. That way no information would be lost while at the same time not surprising clients who only catch URLError based on the documentation.

    At least one exception which I feel must be wrapped is socket.timeout. Since we allow a timeout argument to urlopen, it doesn't make sense for the fact of the timeout to be announced by an exception from another library.

    @pitrou
    Copy link
    Member

    pitrou commented Jan 20, 2012

    Antoine, could "raise ... from" be used here?

    That's a possible compromise indeed. It's up to Senthil to decide
    anyway.

    Perhaps also using new subclasses of URLError to allow the exceptions
    to be caught with finer granularity. That way no information would be
    lost while at the same time not surprising clients who only catch
    URLError based on the documentation.

    I agree there is a problem with the documentation announcing that all
    exceptions will be wrapped. Honestly I would like it better if that
    guarantee were dropped. In the meantime I'm not sure what the best
    course of action is.

    At least one exception which I feel must be wrapped is socket.timeout.
    Since we allow a timeout argument to urlopen, it doesn't make sense
    for the fact of the timeout to be announced by an exception from
    another library.

    You still may be interested to know that the request failed because of a
    timeout rather than (say) an authentication failure, no?

    @orsenthil
    Copy link
    Member

    I have stumbled upon a wrong impression as well by reading the docs, but usually in the code, I tend to catch the specific Exceptions, like timeout instead or URLError when it is known. I saw some libraries following similar pattern too. But that could be changed, if promise in the docs that URLError exception is raised is corrected.

    I think, the course of action for this bug could be.

    1. raise ... from .. for the appropriate Exception in 3.3 and appropriate doc changes.
    2. Doc changes in 2.7,3.2 which say indicate the possibility of other exceptions besides URLError.

    Having any important information in the msg part of the Exception is a bad idea, because it is seldom relied upon and can be changed anytime.

    Shall come out with a patch.

    @vadmium
    Copy link
    Member

    vadmium commented Nov 16, 2015

    There is another patch in duplicate bpo-8823, which has simpler exception wrapping code, but does not add wrapping of reply exceptions.

    Personally, I wouldn’t endorse any new wrapping HTTP client exceptions, and even if wrapping more socket OS errors would be consistent, it could be a backwards compatibility problem. I think the best way forward is documentation: bpo-25633, bpo-22797.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants