Repository: aaugustin/websockets
Pull request #379 (2018-05-01 07:21:43)

Cancel unacknowledged pings.

codecov[bot]: # [Codecov](https://codecov.io/gh/aaugustin/websockets/pull/379?src=pr&el=h1) Report
> Merging [#379](https://codecov.io/gh/aaugustin/websockets/pull/379?src=pr&el=desc) into [master](https://codecov.io/gh/aaugustin/websockets/commit/35d6754ce32f36dab013e0273470be1529578317?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/aaugustin/websockets/pull/379/graphs/tree.svg?height=150&width=650&token=vqr9oExbGY&src=pr)](https://codecov.io/gh/aaugustin/websockets/pull/379?src=pr&el=tree)

```diff
@@          Coverage Diff          @@
##           master   #379   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          29     29           
  Lines        3013   3021    +8     
  Branches      312    313    +1     
=====================================
+ Hits         3013   3021    +8
```


| [Impacted Files](https://codecov.io/gh/aaugustin/websockets/pull/379?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [websockets/test\_protocol.py](https://codecov.io/gh/aaugustin/websockets/pull/379/diff?src=pr&el=tree#diff-d2Vic29ja2V0cy90ZXN0X3Byb3RvY29sLnB5) | `100% <100%> (ø)` | :arrow_up: |
| [websockets/protocol.py](https://codecov.io/gh/aaugustin/websockets/pull/379/diff?src=pr&el=tree#diff-d2Vic29ja2V0cy9wcm90b2NvbC5weQ==) | `100% <100%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/aaugustin/websockets/pull/379?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/aaugustin/websockets/pull/379?src=pr&el=footer). Last update [35d6754...41a957b](https://codecov.io/gh/aaugustin/websockets/pull/379?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/docs/changelog.rst b/docs/changelog.rst
index 745174f..cea1ef4 100644
--- a/docs/changelog.rst
+++ b/docs/changelog.rst
@@ -24,6 +24,9 @@ Also:
 * :func:`~server.unix_serve` can be used as an asynchronous context manager on
   Python ≥ 3.5.1.
 
+* If a :meth:`~protocol.WebSocketCommonProtocol.ping` doesn't receive a pong,
+  it's cancelled when the connection is closed.
+
 4.0
 ...
 
diff --git a/websockets/protocol.py b/websockets/protocol.py
index 1f56693..caf42eb 100644
--- a/websockets/protocol.py
+++ b/websockets/protocol.py
@@ -736,6 +736,19 @@ class WebSocketCommonProtocol(asyncio.StreamReaderProtocol):
             if after_handshake:
                 yield from self.transfer_data_task
 
+            # Cancel all pending pings because they'll never receive a pong.
+            for ping in self.pings.values():
+                ping.cancel()
+            if self.pings:
+                pings_hex = ', '.join(
+                    binascii.hexlify(ping_id).decode() or '[empty]'
+                    for ping_id in self.pings
+                )
+                plural = 's' if len(self.pings) > 1 else ''
+                logger.debug(
+                    "%s - cancelled pending ping%s: %s",
+                    self.side, plural, pings_hex)
+
             # A client should wait for a TCP Close from the server.
             if self.is_client and after_handshake:
                 if (yield from self.wait_for_connection_lost()):


Test patch:
diff --git a/websockets/test_protocol.py b/websockets/test_protocol.py
index bf64b9d..70348fb 100644
--- a/websockets/test_protocol.py
+++ b/websockets/test_protocol.py
@@ -548,6 +548,14 @@ class CommonTests:
         self.run_loop_once()
         self.assertTrue(ping.done())
 
+    def test_cancel_ping(self):
+        ping = self.loop.run_until_complete(self.protocol.ping())
+        # Remove the frame from the buffer, else close_connection() complains.
+        self.last_sent_frame()
+        self.assertFalse(ping.cancelled())
+        self.close_connection()
+        self.assertTrue(ping.cancelled())
+
     def test_acknowledge_previous_pings(self):
         pings = [(
             self.loop.run_until_complete(self.protocol.ping()),
@@ -568,7 +576,7 @@ class CommonTests:
         self.assertTrue(pings[1][0].done())
         self.assertFalse(pings[2][0].done())
 
-    def test_cancel_ping(self):
+    def test_cancelled_ping(self):
         ping = self.loop.run_until_complete(self.protocol.ping())
         ping_frame = self.last_sent_frame()
         ping.cancel()