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

Broken timeout system with ws_connect #8444

Closed
1 task done
arcivanov opened this issue Jun 7, 2024 · 1 comment · Fixed by #8445
Closed
1 task done

Broken timeout system with ws_connect #8444

arcivanov opened this issue Jun 7, 2024 · 1 comment · Fixed by #8445
Labels

Comments

@arcivanov
Copy link
Contributor

arcivanov commented Jun 7, 2024

Describe the bug

Context

We have very specific requirements with our HTTP connections: we expect extremely responsive API and reconnect rapidly if we don't receive responses because we consider it a potential failure.

To facilitate that we use ClientTimeout(connect=1.5, sock_connect=1.0, sock_read=0.5), i.e. yes, if we attempt to read on a socket after sending the request and nothing arrives in 500 ms we consider it a failure and perform a retry.

Problem

This actually works great except when we use the ws_connect for websockets. Having nothing to read for 0.5s of sock_read results in a ServerTimeoutError requiring to retry with ws_connect.

Problem, however, is that ws_connect's timeout and receive_timeout have no effect on the underlying timeout configuration because _ws_connect's call to self.request does not alter the receive timeout for the connection by calling conn.protocol.set_response_params. And after the WSS connection is established, disregarding the ws_connect's receive_timeout the protocol inherits the parent's HTTP connection's sock_read of 0.5.

image

image

image

image

image

To Reproduce

Above

Expected behavior

receive_timeout passed to the ws_connect should be observed, by calling conn.protocol.set_response_params with receive_timeout as read_timeout

Logs/tracebacks

N/A

Python Version

N/A

aiohttp Version

Latest stable

multidict Version

N/A

yarl Version

N/A

OS

OS-independent

Related component

Client

Additional context

No response

Code of Conduct

  • I agree to follow the aio-libs Code of Conduct
@arcivanov arcivanov added the bug label Jun 7, 2024
@arcivanov
Copy link
Contributor Author

I'm going to try to submit a fix.

arcivanov added a commit to arcivanov/aiohttp that referenced this issue Jun 7, 2024
…eouts

Added read_timeout property to ResponseHandler to allow override

After WS(S) connection is established, adjust `conn.proto.read_timeout` to
be the largest of the `read_timeout` and the `ws_connect`'s
`timeout` or `receive_timeout`, whichever are specified.

fixes aio-libs#8444
arcivanov added a commit to arcivanov/aiohttp that referenced this issue Jun 7, 2024
…eouts

Added read_timeout property to ResponseHandler to allow override

After WS(S) connection is established, adjust `conn.proto.read_timeout` to
be the largest of the `read_timeout` and the `ws_connect`'s
`timeout` or `receive_timeout`, whichever are specified.

fixes aio-libs#8444
arcivanov added a commit to arcivanov/aiohttp that referenced this issue Jun 7, 2024
…eouts

Added read_timeout property to ResponseHandler to allow override

After WS(S) connection is established, adjust `conn.proto.read_timeout` to
be the largest of the `read_timeout` and the `ws_connect`'s
`timeout` or `receive_timeout`, whichever are specified.

fixes aio-libs#8444
arcivanov added a commit to arcivanov/aiohttp that referenced this issue Jun 7, 2024
…eouts

Added read_timeout property to ResponseHandler to allow override

After WS(S) connection is established, adjust `conn.proto.read_timeout` to
be the largest of the `read_timeout` and the `ws_connect`'s
`timeout` or `receive_timeout`, whichever are specified.

fixes aio-libs#8444
arcivanov added a commit to arcivanov/aiohttp that referenced this issue Jun 7, 2024
…eouts

Added read_timeout property to ResponseHandler to allow override

After WS(S) connection is established, adjust `conn.proto.read_timeout` to
be the largest of the `read_timeout` and the `ws_connect`'s
`timeout` or `receive_timeout`, whichever are specified.

fixes aio-libs#8444
arcivanov added a commit to arcivanov/aiohttp that referenced this issue Jun 7, 2024
Please see aio-libs#8445 for the source PR
arcivanov added a commit to arcivanov/aiohttp that referenced this issue Jun 7, 2024
…eouts

Added read_timeout property to ResponseHandler to allow override

After WS(S) connection is established, adjust `conn.proto.read_timeout` to
be the largest of the `read_timeout` and the `ws_connect`'s
`timeout` or `receive_timeout`, whichever are specified.

fixes aio-libs#8444
arcivanov added a commit to arcivanov/aiohttp that referenced this issue Jun 7, 2024
…eouts

Added read_timeout property to ResponseHandler to allow override

After WS(S) connection is established, adjust `conn.proto.read_timeout` to
be the largest of the `read_timeout` and the `ws_connect`'s
`timeout` or `receive_timeout`, whichever are specified.

fixes aio-libs#8444
arcivanov added a commit to arcivanov/aiohttp that referenced this issue Jun 8, 2024
Please see aio-libs#8445 for the source PR
arcivanov added a commit to arcivanov/aiohttp that referenced this issue Jun 8, 2024
…eouts

Added read_timeout property to ResponseHandler to allow override

After WS(S) connection is established, adjust `conn.proto.read_timeout` to
be the largest of the `read_timeout` and the `ws_connect`'s
`timeout` or `receive_timeout`, whichever are specified.

fixes aio-libs#8444
arcivanov added a commit to arcivanov/aiohttp that referenced this issue Jun 8, 2024
…eouts

Added read_timeout property to ResponseHandler to allow override

After WS(S) connection is established, adjust `conn.proto.read_timeout` to
be the largest of the `read_timeout` and the `ws_connect`'s
`timeout` or `receive_timeout`, whichever are specified.

fixes aio-libs#8444
arcivanov added a commit to arcivanov/aiohttp that referenced this issue Jun 8, 2024
Added read_timeout property to ResponseHandler to allow override

After WS(S) connection is established, adjust `conn.proto.read_timeout` to
be the largest of the `read_timeout` and the `ws_connect`'s
`ws_timeout` with `None` treated as 'no timeout', i.e. the maximum.

fixes aio-libs#8444
arcivanov added a commit to arcivanov/aiohttp that referenced this issue Jun 8, 2024
Added read_timeout property to ResponseHandler to allow override

After WS(S) connection is established, adjust `conn.proto.read_timeout` to
be the largest of the `read_timeout` and the `ws_connect`'s
`timeout.ws_receive` with `None` treated as 'no timeout', i.e. the maximum.

fixes aio-libs#8444
arcivanov added a commit to arcivanov/aiohttp that referenced this issue Jun 8, 2024
Please see aio-libs#8445 for the source PR
arcivanov added a commit to arcivanov/aiohttp that referenced this issue Jun 8, 2024
Please see aio-libs#8445 for the source PR
arcivanov added a commit to arcivanov/aiohttp that referenced this issue Jun 8, 2024
… not respecting ws_connect's timeouts aio-libs#8444

Please see aio-libs#8445 for the source PR
bdraco added a commit that referenced this issue Jul 17, 2024
…pecting ws_connect's timeouts #8444 (#8447)

Co-authored-by: J. Nick Koston <nick@koston.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant