Today I got burned because I had code that did this: except TimeoutError: When it should have done this: except socket.timeout: There's also another timeout error class in asyncio. Initially I thought, why not make them all use the same exception class? But Guido objects to that: I considered this, and decided against unifying the two TimeoutErrors. First the builtin TimeoutError is specifically a subclass of OSError representing the case where errno is ETIMEDOUT. But asyncio.TimeoutError means nothing of the sort. Second, the precedent is concurrent.futures.TimeoutError. The asyncio one is used under the same conditions as that one. I think we should just update the links in the docs to be correct. So here's my idea: Define a new base class `BaseTimeoutError` and have it be a base class of all other timeout errors. This way people could do: except BaseTimeoutError: And it'll catch all of them. I thought of doing it myself as an abc, but if I remember correctly the except logic ignored __subclasshook__ or __instancecheck__ or whatever is used by abc, so I think this can only work by modifying CPython. What do you think?
On Sat, Apr 01, 2017 at 09:27:34PM +0200, Ram Rachum wrote:
On the face of it, this isn't a serious problem. It seems to me rather like mistakenly writing: except TypeError when you actually intended to write: except ZeroDivisionError You failed to catch the exception that you needed, an got an exception. That's a bug, and the fix for it is to catch the correct exception. It isn't to introduce a new, more complex exception TypeOrZeroDivisionError that potentially catches too much.
There's also another timeout error class in asyncio.
Initially I thought, why not make them all use the same exception class?
That's the wrong way to think about adding classes: Wrong: "Why not add this new class?" Right: "Why add this new class?" We don't start from a position of "Default allow", that is, we add any arbitrary change suggested unless people find a reason to reject it. We start from a conservative position of "Default deny". All changes are rejected, unless they can show enough positive reasons to accept the change. If you start off thinking "Why not make this change?", you will spend a lot of time suggesting things which are rejected. You should spend more time thinking critically about what actual positive reasons: "Why make this change?" What is the benefit of this class? Does it unify two or more *related* exceptions? Guido doesn't think so, and I don't think so either. A socket timeout and an asyncio timeout are, to my mind, *unrelated*. Just because they both relate to the passage of time doesn't make them the same thing.
I agree with Guido's analysis. I don't think that the connection between the two timeout exceptions are related.
Why would you do that? This is exactly what I mean when I say that we need to have positive reasons for a change. It isn't enough to say that we can catch "all of them". We can already do that: # Did I miss any? except (TimeoutError, asyncio.TimeoutError, concurrent.futures.TimeoutError, multiprocessing.TimeoutError, socket.timeout, subprocess.TimeoutExpired): but *why* would I write that code? Under what circumstances will I have something that could raise *any* of those exceptions, and I will want to treat them *all* the same way? It seems far more likely that I would *not* want to treat them the same way. If I have some networking code that raises socket.timeout, I probably want to catch that exception and try again (maybe with backoff). But if that same piece of networking code were to raise concurrent.futures.TimeoutError or subprocess.TimeoutExpired, I'd want to know about it! I'd want to see the exception, which means *not* catching it. Or the other way around... if I am working with Futures, and have to deal with futures.TimeoutError, that doesn't mean that a networking timeout or a system call timeout should be treated the same way. I'm having difficulty in seeing any positive benefit to this proposed base class. If you really want this, you can name a tuple of all the unrelated exceptions you want to catch: TimeoutErrors = (TimeoutError, asyncio.TimeoutError, concurrent.futures.TimeoutError, socket.timeout, multiprocessing.TimeoutError, subprocess.TimeoutExpired) try: ... except TimeoutErrors: ... but I cannot possibly recommend that. To me, that looks like it will catch too much under nearly all reasonable circumstances. -- Steve
On Sun, Apr 2, 2017 at 1:11 PM, Steven D'Aprano <steve@pearwood.info> wrote:
What I'd like to see is a linter/static analyzer that can look at your code and determine heuristically that it is highly unlikely for this exception to be raised. With some exceptions, the assumption is "can be raised anywhere", but with custom or library-specific exceptions, only a raise statement can cause them. If you had a linter like that, you could spot the wrong timeout being caught, as it would notify you that there's no (normal) way for that to catch anything. Note that I am adamantly NOT suggesting this as a language feature. A linter is allowed to be wrong occasionally if it's helpful the rest of the time, and it's allowed to ignore the possibilities of weird aliasing or monkey-patching. Does this already exist somewhere? Is there something people would recommend? ChrisA
On 4/1/2017 3:27 PM, Ram Rachum wrote:
Both are subclasses of OSError but mean different things. TimeoutError means that something in 'your system' did not respond. Socket.timeout means that a foreign system did not respond. (I am leaving out a local socket connection.) The latter can only happen with a socket call with timeouts enabled. If it is possible for TimeoutError to also occur with a timeout-enabled socket call, then one might very well want to only catch one or catch them separately and respond differently.
There's also another timeout error class in asyncio.
Which can only happen when using asyncio. -1 on merging, +1 on staying with the current design. What I write above is similar to Guido's explanation for asyncio.TimeoutError. -- Terry Jan Reedy
On Sun, Apr 2, 2017 at 4:08 PM, Terry Reedy <tjreedy@udel.edu> wrote:
I don't necessarily disagree with the conclusion, but I'm pretty sure this is rationale is wrong: connect / recv / etc. can return ETIMEDOUT if the peer is non-responsive and some internal operating-system-specific timeout expires (see e.g. [1], or the TCP_USER_TIMEOUT socket option). So both TimeoutError and socket.timeout can indicate a non-responsive remote system, and the actual difference is in who implements the timeout: socket.timeout means that the timeout implemented by the Python interpreter and controlled by socket.settimeout() expired; TimeoutError means that the timeout implemented by the kernel expired. You can also get TimeoutError in other odd places like the pthread synchronization functions that take a timeout, or when working with POSIX message queues for IPC. These do describe stuff that's on the same system timing out, but since these APIs aren't exposed by Python I'm not actually sure if it's possible in practice to get a TimeoutError except with a socket talking to a non-responsive peer. -n [1] https://stackoverflow.com/questions/8471577/linux-tcp-connect-failure-with-e... -- Nathaniel J. Smith -- https://vorpus.org
On Sat, Apr 01, 2017 at 09:27:34PM +0200, Ram Rachum wrote:
On the face of it, this isn't a serious problem. It seems to me rather like mistakenly writing: except TypeError when you actually intended to write: except ZeroDivisionError You failed to catch the exception that you needed, an got an exception. That's a bug, and the fix for it is to catch the correct exception. It isn't to introduce a new, more complex exception TypeOrZeroDivisionError that potentially catches too much.
There's also another timeout error class in asyncio.
Initially I thought, why not make them all use the same exception class?
That's the wrong way to think about adding classes: Wrong: "Why not add this new class?" Right: "Why add this new class?" We don't start from a position of "Default allow", that is, we add any arbitrary change suggested unless people find a reason to reject it. We start from a conservative position of "Default deny". All changes are rejected, unless they can show enough positive reasons to accept the change. If you start off thinking "Why not make this change?", you will spend a lot of time suggesting things which are rejected. You should spend more time thinking critically about what actual positive reasons: "Why make this change?" What is the benefit of this class? Does it unify two or more *related* exceptions? Guido doesn't think so, and I don't think so either. A socket timeout and an asyncio timeout are, to my mind, *unrelated*. Just because they both relate to the passage of time doesn't make them the same thing.
I agree with Guido's analysis. I don't think that the connection between the two timeout exceptions are related.
Why would you do that? This is exactly what I mean when I say that we need to have positive reasons for a change. It isn't enough to say that we can catch "all of them". We can already do that: # Did I miss any? except (TimeoutError, asyncio.TimeoutError, concurrent.futures.TimeoutError, multiprocessing.TimeoutError, socket.timeout, subprocess.TimeoutExpired): but *why* would I write that code? Under what circumstances will I have something that could raise *any* of those exceptions, and I will want to treat them *all* the same way? It seems far more likely that I would *not* want to treat them the same way. If I have some networking code that raises socket.timeout, I probably want to catch that exception and try again (maybe with backoff). But if that same piece of networking code were to raise concurrent.futures.TimeoutError or subprocess.TimeoutExpired, I'd want to know about it! I'd want to see the exception, which means *not* catching it. Or the other way around... if I am working with Futures, and have to deal with futures.TimeoutError, that doesn't mean that a networking timeout or a system call timeout should be treated the same way. I'm having difficulty in seeing any positive benefit to this proposed base class. If you really want this, you can name a tuple of all the unrelated exceptions you want to catch: TimeoutErrors = (TimeoutError, asyncio.TimeoutError, concurrent.futures.TimeoutError, socket.timeout, multiprocessing.TimeoutError, subprocess.TimeoutExpired) try: ... except TimeoutErrors: ... but I cannot possibly recommend that. To me, that looks like it will catch too much under nearly all reasonable circumstances. -- Steve
On Sun, Apr 2, 2017 at 1:11 PM, Steven D'Aprano <steve@pearwood.info> wrote:
What I'd like to see is a linter/static analyzer that can look at your code and determine heuristically that it is highly unlikely for this exception to be raised. With some exceptions, the assumption is "can be raised anywhere", but with custom or library-specific exceptions, only a raise statement can cause them. If you had a linter like that, you could spot the wrong timeout being caught, as it would notify you that there's no (normal) way for that to catch anything. Note that I am adamantly NOT suggesting this as a language feature. A linter is allowed to be wrong occasionally if it's helpful the rest of the time, and it's allowed to ignore the possibilities of weird aliasing or monkey-patching. Does this already exist somewhere? Is there something people would recommend? ChrisA
On 4/1/2017 3:27 PM, Ram Rachum wrote:
Both are subclasses of OSError but mean different things. TimeoutError means that something in 'your system' did not respond. Socket.timeout means that a foreign system did not respond. (I am leaving out a local socket connection.) The latter can only happen with a socket call with timeouts enabled. If it is possible for TimeoutError to also occur with a timeout-enabled socket call, then one might very well want to only catch one or catch them separately and respond differently.
There's also another timeout error class in asyncio.
Which can only happen when using asyncio. -1 on merging, +1 on staying with the current design. What I write above is similar to Guido's explanation for asyncio.TimeoutError. -- Terry Jan Reedy
On Sun, Apr 2, 2017 at 4:08 PM, Terry Reedy <tjreedy@udel.edu> wrote:
I don't necessarily disagree with the conclusion, but I'm pretty sure this is rationale is wrong: connect / recv / etc. can return ETIMEDOUT if the peer is non-responsive and some internal operating-system-specific timeout expires (see e.g. [1], or the TCP_USER_TIMEOUT socket option). So both TimeoutError and socket.timeout can indicate a non-responsive remote system, and the actual difference is in who implements the timeout: socket.timeout means that the timeout implemented by the Python interpreter and controlled by socket.settimeout() expired; TimeoutError means that the timeout implemented by the kernel expired. You can also get TimeoutError in other odd places like the pthread synchronization functions that take a timeout, or when working with POSIX message queues for IPC. These do describe stuff that's on the same system timing out, but since these APIs aren't exposed by Python I'm not actually sure if it's possible in practice to get a TimeoutError except with a socket talking to a non-responsive peer. -n [1] https://stackoverflow.com/questions/8471577/linux-tcp-connect-failure-with-e... -- Nathaniel J. Smith -- https://vorpus.org
participants (5)
-
Chris Angelico -
Nathaniel Smith -
Ram Rachum -
Steven D'Aprano -
Terry Reedy