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.

Sign up for GitHub

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

Jump to bottom

websockets: fix ping_timeout #3376

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

websockets: fix ping_timeout #3376

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions tornado/test/websocket_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -806,7 +806,11 @@ class PingHandler(TestWebSocketHandler):
def on_pong(self, data):
self.write_message("got pong")

return Application([("/", PingHandler)], websocket_ping_interval=0.01)
return Application(
[("/", PingHandler)],
websocket_ping_interval=0.01,
websocket_ping_timeout=1,
)

@gen_test
def test_server_ping(self):
Expand All @@ -827,7 +831,7 @@ def on_ping(self, data):

@gen_test
def test_client_ping(self):
ws = yield self.ws_connect("/", ping_interval=0.01)
ws = yield self.ws_connect("/", ping_interval=0.01, ping_timeout=1)
for i in range(3):
response = yield ws.read_message()
self.assertEqual(response, "got ping")
Expand Down
34 changes: 12 additions & 22 deletions tornado/websocket.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -835,7 +835,6 @@ def __init__(
self._wire_bytes_in = 0
self._wire_bytes_out = 0
self.ping_callback = None # type: Optional[PeriodicCallback]
self.last_ping = 0.0
self.last_pong = 0.0
self.close_code = None # type: Optional[int]
self.close_reason = None # type: Optional[str]
Expand Down Expand Up @@ -1303,38 +1302,29 @@ def start_pinging(self) -> None:
"""Start sending periodic pings to keep the connection alive"""
assert self.ping_interval is not None
if self.ping_interval > 0:
self.last_ping = self.last_pong = IOLoop.current().time()
self.ping_callback = PeriodicCallback(
self.periodic_ping, self.ping_interval * 1000
)
self.ping_callback.start()

def periodic_ping(self) -> None:
"""Send a ping to keep the websocket alive
async def periodic_ping(self) -> None:
"""Send a ping and wait for a pong if ping_timeout is configured.

Called periodically if the websocket_ping_interval is set and non-zero.
"""
if self.is_closing() and self.ping_callback is not None:
self.ping_callback.stop()
return

# Check for timeout on pong. Make sure that we really have
# sent a recent ping in case the machine with both server and
# client has been suspended since the last ping.
now = IOLoop.current().time()
since_last_pong = now - self.last_pong
since_last_ping = now - self.last_ping
assert self.ping_interval is not None
assert self.ping_timeout is not None
if (
since_last_ping < 2 * self.ping_interval
and since_last_pong > self.ping_timeout
):
self.close()
return

# send a ping
self.write_ping(b"")
self.last_ping = now

if self.ping_timeout and self.ping_timeout > 0:
# wait for the pong
await asyncio.sleep(self.ping_timeout)
Copy link
Member

Choose a reason for hiding this comment

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

Unconditionally sleeping for ping_timeout will cause problems if ping_timeout is greater than ping_interval (which is the default) - we won't start the next ping until the old timeout sleep has passed.

Unless we want to effectively require that ping_interval >= ping_timeout, we can't use a sleep here. (Since websockets are TCP, I don't think it really makes sense to use a short ping interval with a long timeout, so maybe we'd want to make that policy change, but we have the existing defaults and documentation that suggest otherwise). We could use other synchronization primitives (like an asyncio.Condition) that would allow for an early wakeup, or we could go back to the original pattern that checks for a timeout at the ping interval.

I also find the combination of PeriodicCallback and coroutines kind of non-obvious and if we're going to use a coroutine here I think I'd rather get rid of the PeriodicCallback and send the pings from the same coroutine. This would make the interaction of the sending of pings and waiting for pongs more explicit.


# close the connection if the pong is not received within the
# configured timeout
if self.last_pong - now > self.ping_timeout:
self.close()

def set_nodelay(self, x: bool) -> None:
self.stream.set_nodelay(x)
Expand Down