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

ClientTimeout has 4 parameters, but they all throw the same exception #7161

Closed
1 task done
tgm-git opened this issue Jan 9, 2023 · 5 comments
Closed
1 task done

Comments

@tgm-git
Copy link

tgm-git commented Jan 9, 2023

Is your feature request related to a problem?

I am using aiohttp in a small liveness-checker utility tool. The tool will connect to a site to determine if the link is still active. Earlier versions used requests, where the various timeouts have different exceptions. This is very useful to end the connection early once the server has responded. The actual content of the page is not relevant for my use-case so waiting to download slows the tool significantly.

To summarise, my tool needs to behave differently if the timeout is caused by sock_connect or sock_read.

Describe the solution you'd like

Four new Exceptions deriving from TimeoutError:

  • TotalTimeOutError
  • ConnectTimeoutError
  • SockReadTimeoutError
  • SockConnectTimeoutError

Where each is exception is thrown when the related timeout is exceeded. The names are just suggestions.

Describe alternatives you've considered

I have used requests earlier where the different timeouts (not 1-1 with timeouts in aiohttp) have different exceptions. However, running requests asynchronously is cumbersome compared to aiohttp.

Related component

Client

Additional context

No response

Code of Conduct

  • I agree to follow the aio-libs Code of Conduct
@webknjaz webknjaz changed the title ClinetTimeout has 4 parameters, but they all throw the same exception ClientTimeout has 4 parameters, but they all throw the same exception Jan 9, 2023
@Dreamsorcerer
Copy link
Member

If I understand that correctly, you want to do something along the lines of retrying if sock_connect failed, but continue as completed if sock_read failed?

Seems reasonable, if you'd like to make a PR. I wonder if adding an attribute to TimeoutError might be a better solution though, rather than creating several more exception classes.

@webknjaz
Copy link
Member

webknjaz commented Jan 9, 2023

Usually, having to inspect the exception's metadata makes the calling code more messy. It seems reasonable to have exceptions that represent distinct high-level states that could be processed explicitly. Readability-wise.
Those new exceptions should also make use of multiple inheritance, similar to what Python 3 has now in terms of ConnectionError subclasses: https://docs.python.org/3/library/exceptions.html#ConnectionError. Back in the day, it used to be an ugly socket.error that turned into an alias for OSError over time, encapsulating too much, semantically. But new exceptions like BrokenPipeError (errno.EPIPE and errno.ESHUTDOWN), ConnectionAbortedError (errno.ECONNABORTED), ConnectionRefusedError (errno.ECONNREFUSED), ConnectionResetError (errno.ECONNRESET).
So I'd expect these new exceptions to follow the example of stdlib in naming and granularity. Also, maybe in some cases, we should just throw those stdlib exceptions.

@tgm-git
Copy link
Author

tgm-git commented Jan 10, 2023

From my use-case's point of view catching the different exceptions in additional except blocks would be cleaner than handling exception metadata.

I will have a look at creating a PR over the weekend.

@tgm-git
Copy link
Author

tgm-git commented Jan 18, 2023

I have not had time to start the pull request, but it is on my todo list. If anyone else has the same problem feel free to start a pull-request and tag me in it.

@Dreamsorcerer
Copy link
Member

As per #7122 (comment) this appears to already be done.

If you only get TimeoutError in all situations, you'll need to provide a reproducer of the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants