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

Integrate websocket periodic pinging code from Jupyter #1642

Closed
wants to merge 1 commit into from

Conversation

takluyver
Copy link
Contributor

Closes gh-1640

As Ben requested on #1640, I've changed it to work in seconds rather than milliseconds.

I'm not sure how we'd test something like this. I don't think we have tests for it in Jupyter.

Closes tornadowebgh-1640

As Ben requested on tornadoweb#1640, I've changed it to work in seconds rather
than milliseconds.

I'm not sure how we'd test something like this. I don't think we have
tests for it in Jupyter.
Copy link
Member

@bdarnell bdarnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, and sorry for letting this sit neglected for a year. I'll pull it locally and make a couple of changes before merging. I'm not sure how to test this either without a lot of extra machinery.

since_last_pong = now - self.last_pong
since_last_ping = now - self.last_ping
if since_last_ping < 2*self.ping_interval and since_last_pong > self.ping_timeout:
self.log.warn("WebSocket ping timeout after %i ms.", since_last_pong)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no such thing as self.log.warn here.

tornado's regular pinging may decide that the connection has dropped
and close the websocket.
"""
self.last_pong = IOLoop.current().time()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to do this outside the overridable method so there's no question about calling the superclass method.

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

Successfully merging this pull request may close these issues.

2 participants