Skip to content

Ping at scheduled intervals #383

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

Closed
aaugustin opened this issue May 2, 2018 · 14 comments
Closed

Ping at scheduled intervals #383

aaugustin opened this issue May 2, 2018 · 14 comments

Comments

@aaugustin
Copy link
Member

We could add a ping_interval argument to the protocol.

When set, the protocol would start a task to send a ping at regular intervals.

This can help to keep connections open, especially across proxies with short timeouts.

This task would be a simple as:

while self.open:
    yield from asyncio.sleep(self.ping_interval)
    yield from self.ping()

As a bonus, we could kill the connection if no pong is received in one ping_interval.

@bmoscon
Copy link

bmoscon commented May 3, 2018

I'd love to see this - I did something very similar for a project that uses this library, so its definitely useful

@RemiCardona
Copy link
Collaborator

On our project, we already have something really similar, but with automatic reconnection. I fail to see how I could reuse what is proposed here without reimplementing the whole method/function to fit out own needs.

Don't get me wrong, I'm not opposed to websockets having support for something like this, I just wanted to say that, in my case, I most likely wouldn't be able to use it as proposed. However, I definitely see use for this.

Cheers

@cjerdonek
Copy link
Collaborator

As an alternative, since it's so simple, it might be useful to start out by documenting the pattern. Also, for people calling a websockets server from a browser, the same pattern using Javascript would be similarly useful.

@RemiCardona
Copy link
Collaborator

While the ping mechanism is officially part of the protocol spec, there's no official API to access it from the JS/browser side. It's up to browsers to figure if/when to initiate pings or pongs to check the connection status.

https://developer.mozilla.org/en-US/docs/Web/API/WebSocket

@cjerdonek
Copy link
Collaborator

Yes, I was just suggesting that, if sample code is provided to show how to ping at regular intervals using Python to keep a connection alive, then it could also be useful to provide such sample code for Javascript -- even if there is no "canonical" solution due to the lack of an API.

@aaugustin
Copy link
Member Author

@RemiCardona To signal to the application that the connection is lost, we can provide a Future and complete it when a ping times out. Then it's up to the application to close and reopen the connection (only on the client side).

I think we don't enforce enough structure to handle this automatically.

I don't believe we can inject an exception either — see #338 for a discussion of cancelling the connection handler (on the server side) when the connection is close, a more basic situation than the one we're discussing here.

@vgoklani
Copy link

Hey - has this been implemented? Please add :) Thanks!

@aaugustin
Copy link
Member Author

This issue is open, which means this feature hasn't been implemented yet.

@aaugustin
Copy link
Member Author

I proposed an implementation of this feature in #449.

It only deals with sending pings regularly, not with automatic reconnection — that's #414. However, if automatic reconnection is implemented, this feature will make it more reliable because it will detect connection failures more eagerly.

@aaugustin
Copy link
Member Author

I would very much appreciate feedback on my PR, especially if you can test it in realistic circumstances and report whether it behaves cleanly.

@vgoklani
Copy link

testing now :) how do I change the ping message?

@aaugustin
Copy link
Member Author

The ping message is a random 4-byte value. This allows matching pongs to pings.

@aaugustin
Copy link
Member Author

(You can send a ping with a specific payload if you want to with the ping API, but that's another story.)

aaugustin added a commit that referenced this issue Jul 30, 2018
aaugustin added a commit that referenced this issue Aug 20, 2018
aaugustin added a commit that referenced this issue Aug 20, 2018
@aaugustin
Copy link
Member Author

I added tests and improved the pull request. I think it's ready to merge.

I'm still looking for a code review and/or reports of whether it works well.

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

5 participants