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

[QUESTION] WebSocketResponse timeout parameter #1325

Closed
Marco-Sulla opened this issue Oct 21, 2016 · 7 comments
Closed

[QUESTION] WebSocketResponse timeout parameter #1325

Marco-Sulla opened this issue Oct 21, 2016 · 7 comments
Labels

Comments

@Marco-Sulla
Copy link

WebSocketResponse timeout parameter is not documented:
https://aiohttp.readthedocs.io/en/stable/web_reference.html#websocketresponse

What does it do? It's a timeout for websocket creation or for websocket messages? From source code, it seems it involves websocket closing only.

@asvetlov
Copy link
Member

You've raised very interesting and important question.
Yes, timeout should be enforced on all websocket operations.
Maybe 10 secs is too low value.
Documentation should be updated also to encourage sending pings (either WS PING frames or custom data packets).
It may solve very annoying issue when socket is not active actually but no disconnection message was arrived.

The story is waiting for a hero.
Unfortunately right now I'm working on other issues: YARL incorporating and nested sub-applications.

@brentspell
Copy link

It appears that the websocket timeout is not being honored during reads.

https://github.com/KeepSafe/aiohttp/blob/master/aiohttp/client_ws.py#L129

This can be worked around by wrapping the receive*_ call with asyncio.wait_for, but the websocket client should do this internally.

@fafhrd91
Copy link
Member

Good catch. Question should we use two separate timeout parameters, one for receive and another for close or same for both?

@brentspell
Copy link

That is a good question. It would probably make sense to use different timeouts for close/receive. However, for backward compatibility, the existing timeout parameter would need to be used for the close timeout, since the current receive timeout is effectively infinite. So, creating a new receive_timeout parameter with a default of None (infinite) would probably be the safest approach.

It would also be useful to allow passing a timeout to the receive_* function that could override the default. This would be analogous to the timeout parameter in ClientSession.request.

@rutsky
Copy link
Member

rutsky commented Jan 31, 2017

Related: #777 , #1024 .

@fafhrd91
Copy link
Member

added receive_timeout to server and client in master

@lock
Copy link

lock bot commented Oct 29, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

5 participants