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

Replace custom exceptions for timeouts with TimeoutError #86579

Closed
tiran opened this issue Nov 19, 2020 · 20 comments
Closed

Replace custom exceptions for timeouts with TimeoutError #86579

tiran opened this issue Nov 19, 2020 · 20 comments
Assignees
Labels
3.11 expert-asyncio extension-modules stdlib type-feature

Comments

@tiran
Copy link
Member

@tiran tiran commented Nov 19, 2020

BPO 42413
Nosy @brianquinlan, @pitrou, @vstinner, @tiran, @alex, @asvetlov, @serhiy-storchaka, @1st1, @dstufft, @applio, @corona10, @miss-islington, @kumaraditya303
PRs
  • #23413
  • #23520
  • #30197
  • 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/asvetlov'
    closed_at = <Date 2021-12-19.11:38:06.321>
    created_at = <Date 2020-11-19.19:10:42.609>
    labels = ['extension-modules', '3.11', 'type-feature', 'library', 'expert-asyncio']
    title = 'Replace custom exceptions for timeouts with TimeoutError'
    updated_at = <Date 2021-12-19.11:38:06.320>
    user = 'https://github.com/tiran'

    bugs.python.org fields:

    activity = <Date 2021-12-19.11:38:06.320>
    actor = 'asvetlov'
    assignee = 'asvetlov'
    closed = True
    closed_date = <Date 2021-12-19.11:38:06.321>
    closer = 'asvetlov'
    components = ['Extension Modules', 'Library (Lib)', 'asyncio']
    creation = <Date 2020-11-19.19:10:42.609>
    creator = 'christian.heimes'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 42413
    keywords = ['patch']
    message_count = 20.0
    messages = ['381454', '381466', '381539', '381552', '381553', '381554', '381555', '381556', '381557', '381558', '381559', '381560', '381562', '381568', '381663', '381664', '381886', '404693', '408893', '408894']
    nosy_count = 14.0
    nosy_names = ['bquinlan', 'janssen', 'pitrou', 'vstinner', 'christian.heimes', 'alex', 'asvetlov', 'serhiy.storchaka', 'yselivanov', 'dstufft', 'davin', 'corona10', 'miss-islington', 'kumaraditya']
    pr_nums = ['23413', '23520', '30197']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue42413'
    versions = ['Python 3.11']

    @tiran
    Copy link
    Member Author

    @tiran tiran commented Nov 19, 2020

    The socket module has a custom timeout exception "socket.timeout". The exception is documented https://docs.python.org/3/library/socket.html#socket.timeout as :

    A subclass of OSError, this exception is raised when a timeout occurs on a socket which has had timeouts enabled via a prior call to settimeout() (or implicitly through setdefaulttimeout()).

    It's a distinct exception type and not the same as the global TimeoutError.

    >>> import socket
    >>> socket.timeout == TimeoutError
    False

    I like to follow the example of the deprecated "socket.error", replace the custom exception and make it an alias of TimeoutError. The risk is minimal. Both exceptions are subclasses of OSError.

    The change would not only make exception handling more consistent. It would also allow me to simplify some code in ssl and socket C code. I might even be able to remove the socket CAPI import from _ssl.c completely.

    @tiran tiran self-assigned this Nov 19, 2020
    @tiran tiran added 3.10 extension-modules type-feature labels Nov 19, 2020
    @miss-islington
    Copy link
    Contributor

    @miss-islington miss-islington commented Nov 20, 2020

    New changeset 03c8ddd by Christian Heimes in branch 'master':
    bpo-42413: socket.timeout is now an alias of TimeoutError (GH-23413)
    03c8ddd

    @tiran tiran closed this as completed Nov 20, 2020
    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Nov 21, 2020

    There are also other distinct exceptions:

    multiprocessing.TimeoutError
    concurrent.futures.TimeoutError
    asyncio.TimeoutError

    @tiran
    Copy link
    Member Author

    @tiran tiran commented Nov 21, 2020

    Thanks Serhiy!

    Andrew, Antoine, Yury, do you want to replace the custom Timeout exceptions in asyncio, multiprocessing, and concurrent with global TimeoutError, too?

    @tiran tiran reopened this Nov 21, 2020
    @asvetlov
    Copy link
    Contributor

    @asvetlov asvetlov commented Nov 21, 2020

    In futures and asyncio TimeoutError has no errno.

    I'm not sure should we care but I consider it as a show stopper.

    On another hand, two distinct timeout classes confuse people. I had many conversions about this during the aiohttp support. asyncio can raise both TimeoutError and asyncio.TimeoutError which is... unexpected at least.

    @tiran
    Copy link
    Member Author

    @tiran tiran commented Nov 21, 2020

    PyErr_SetString(PyErr_TimeoutError, "msg") and raise TimeoutError("msg") do not set any additional exception attributes. errno, strerror, filename, and filename2 are None:

    >>> try:
    ...     raise TimeoutError("msg")
    ... except Exception as e:
    ...     err = e
    ... 
    >>> err
    TimeoutError('msg')
    >>> (err.errno, err.filename, err.filename2, err.strerror)
    (None, None, None, None)

    Is that a problem?

    @asvetlov
    Copy link
    Contributor

    @asvetlov asvetlov commented Nov 21, 2020

    Perhaps it is a good compromise.
    OSError-derived class without errno looks getter to me that different incompatible TimeoutError classes.

    How many exceptions inherited from OSError have no errno set? Do we have a precedent in stdlib at all already?

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Nov 21, 2020

    Yes, I think this is a problem. It is expected that an instance of OSError has valid errno. OSError is a merge of several old exception types, some of them did have errno, and others did not.

    Maybe add BaseTimeoutError and make TimeoutError a subclass of BaseTimeoutError and OSError?

    There as also several CancelledError, and exceptions with similar names and purpose, like asyncio.IncompleteReadError and http.client.IncompleteRead.

    @tiran
    Copy link
    Member Author

    @tiran tiran commented Nov 21, 2020

    They have an errno attribute, it's just to None by default. You have to call OSError with multiple arguments to set errno to a non-None value.

    @asvetlov
    Copy link
    Contributor

    @asvetlov asvetlov commented Nov 21, 2020

    I know that I just create OSError() with errno set to None.

    My question is: has the standard library such code examples already?
    In other words, how many third-party code will be broken by catching OSError with errno=None?

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Nov 21, 2020

    I fixed many instantiations of OSError subclasses, but many are still left in Python code.

    @asvetlov
    Copy link
    Contributor

    @asvetlov asvetlov commented Nov 21, 2020

    Thus using bare TimeoutError in asyncio is safe, isn't it?
    This is good news!

    @tiran
    Copy link
    Member Author

    @tiran tiran commented Nov 21, 2020

    There is another option: The subclasses of OSError could set a correct errno automatically.

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Nov 21, 2020

    This is a good idea. Writing IsADirectoryError(errno.EISDIR, os.strerror(errno.EISDIR), filename) is boring.

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Nov 23, 2020

    IMHO it's ok that exc.errno is None. It doesn't prevent to write code like:

    except OSError as exc:
        if exc.errno == ...:
            ...
        else:
            ...

    In the early days (first 5 years? :-D) of the asyncio documentation, TimeoutError was documented just as "TimeoutError", instead of "asyncio.TimeoutError". So if you followed carefully the asyncio documentation and wrote "except TimeoutError:", the except would never be reached beause asyncio.TimeoutError is *not* a subclass of the builtin Timeout...

    >>> issubclass(asyncio.TimeoutError, TimeoutError)
    False

    It would be great to have a single TimeoutError class. I'm fine with having weird attributes depending who raise the exception. Honestly, in most cases "except TimeoutError:" is enough: there is no need to check for exception attributes.

    @vstinner
    Copy link
    Member

    @vstinner vstinner commented Nov 23, 2020

    I see this issue as a follow-up of PEP-3151 which started to uniformize IOError, OSError and many variants that we had in Python 2.

    socket.timeout was introduced as a subclass of TimeoutError according to:
    https://www.python.org/dev/peps/pep-3151/#new-exception-classes

    In Python 3.9, socket.timeout and TimeoutError are subclasses of OSError but are different. I agree that it was confusion.

    @asvetlov asvetlov changed the title Replace custom exception socket.timeout with TimeoutError Replace custom exceptions for timeouts with TimeoutError Nov 26, 2020
    @asvetlov asvetlov added stdlib expert-asyncio labels Nov 26, 2020
    @asvetlov
    Copy link
    Contributor

    @asvetlov asvetlov commented Nov 26, 2020

    Pull Request #23520 applies the discussed change to both asyncio and concurrent.futures.

    I did the minimally invasive change, libraries still use asyncio.TimeoutError and concurrent.futures.TimeoutError internally but both names are aliases now.

    @tiran
    Copy link
    Member Author

    @tiran tiran commented Oct 21, 2021

    Andrew, could you please rebase your PR and get it submitted?

    @tiran tiran added 3.11 and removed 3.10 labels Oct 21, 2021
    @tiran tiran removed their assignment Oct 21, 2021
    @asvetlov
    Copy link
    Contributor

    @asvetlov asvetlov commented Dec 19, 2021

    New changeset da4b214 by Kumar Aditya in branch 'main':
    bpo-42413: Replace concurrent.futures.TimeoutError and asyncio.TimeoutError with builtin TimeoutError (GH-30197)
    da4b214

    @asvetlov
    Copy link
    Contributor

    @asvetlov asvetlov commented Dec 19, 2021

    Done.
    Thanks, Kumar!

    @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
    3.11 expert-asyncio extension-modules stdlib type-feature
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants