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

ClientWebSocket heartbeat should throw exceptions #4153

Closed
alandtse opened this issue Oct 7, 2019 · 12 comments · Fixed by #8685
Closed

ClientWebSocket heartbeat should throw exceptions #4153

alandtse opened this issue Oct 7, 2019 · 12 comments · Fixed by #8685

Comments

@alandtse
Copy link
Contributor

alandtse commented Oct 7, 2019

Long story short

A client websocket session with a heartbeat enabled will never indicate the connection is closed if a program is only receiving information.

This may be related to #2309 (which was server-side, but others appear to be commenting about client-side), where @asvetlov stated:

Workaround (the proper solution actually) is sending ping messages from a client.
Unfortunately, browsers don't support WebSocket pings, you have to emulate them by small regular messages.

My understanding is the heartbeat is intended to create the ping message to detect the disconnection. Therefore, when someone is using the heartbeat, an exception should be raised when pong not received.

Expected behaviour

The Timeout exception should be raised upon detecting that _pong_not_received for calling programs to catch.

Actual behaviour

The connection closes silently. The only way to detect it would be to add additional logic to send a ping, which is the purpose of the heartbeat.

Steps to reproduce

  1. Create a receive only websocket connection to any server and enable a heartbeat
async with session.ws_connect('http://example.org/ws', heartbeat=5) as ws:
    async for msg in ws:
        _LOGGER.debug("msg: %s", msg)
  1. Simulate a disconnect by disconnecting internet.
  2. Websocket messages will stop because the connection is closed. However, this is silent.

Your environment

Client
OS isn't relevant

@alandtse
Copy link
Contributor Author

alandtse commented Oct 8, 2019

So the solution is that upon exiting the async for msg in ws: loop, you can check the websocket's close reasons including ws.exception(). That will contain the concurrent.futures._base.TimeoutError.

@alandtse alandtse closed this as completed Oct 8, 2019
@asvetlov asvetlov reopened this Oct 8, 2019
@asvetlov
Copy link
Member

asvetlov commented Oct 8, 2019

let me meditate on it

@ghost
Copy link

ghost commented Sep 7, 2020

@asvetlov Any update on this?

I came across the same problem as @alandtse, however the solution they provided does not properly address the real problem, as one will only exit the for loop if the connection comes UP again (otherwise it will just hang), which again defies the whole point of having a heartbeat.
I do agree however wih the 'Expected Behaviour' they mentioned on their first comment.

@ghost
Copy link

ghost commented Sep 19, 2020

I have a fix for this issue that works correctly with aiohttp3.6.2 and Python 3.8.5, however aiottp 4.0.0a1 introduces another bug: #4526 that supersedes my fix.

Are you still maintaining the stable version or only working on 4.0.*?

@asvetlov
Copy link
Member

aiohttp 3.7 is prepared to release; aiohttp 4.0 will probably have reimplemented heartbeats

@jhaenchen
Copy link

Where'd we end up here?

@Mirraz
Copy link

Mirraz commented Aug 4, 2022

Have the same issue: heartbeat is on, underlying TCP-connection appeared blocked, _pong_not_received is called but main code still awaits for messages from web-socket

@socketpair
Copy link
Contributor

@alandtse @asvetlov @gholt
Finally, is it fixed or not ? some intersecting issues.. If yes, in what version ?

@socketpair
Copy link
Contributor

socketpair commented Aug 11, 2022

@alandtse seems your explanation is not correct. you said:

Websocket messages will stop because the connection is closed. However, this is silent.

but actually, reading will not be stopped.

@rgeronimi
Copy link

Same issue met here. This is pretty critical as the server maintains resources after the client has vanished, and the heartbeat does not raise an exception about that.

@Dreamsorcerer
Copy link
Member

@bdraco Does any of your recent fixes affect this issue or #2309?

@bdraco
Copy link
Member

bdraco commented Aug 17, 2024

Its partially fixed. #8685 should fix the rest of the cases

bdraco pushed a commit that referenced this issue Aug 17, 2024
… task not being consumed (#8729)

Co-authored-by: J. Nick Koston <nick@koston.org>
closes #7238
fixes #5182
fixes #4153
fixes #2309
bdraco pushed a commit that referenced this issue Aug 17, 2024
… task not being consumed (#8730)

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

Successfully merging a pull request may close this issue.

8 participants