diff mbox series

[v3,1/1] python-websockets: backport fix for upstream issue #350

Message ID 20180510221712.13031-1-joseph.kogut@gmail.com
State Accepted
Commit 08753b088909b447f989f9d211c74a18bb677294
Headers show
Series [v3,1/1] python-websockets: backport fix for upstream issue #350 | expand

Commit Message

Joseph Kogut May 10, 2018, 10:17 p.m. UTC
Signed-off-by: Joseph Kogut <joseph.kogut@gmail.com>

Changes v2 -> v3:
	- Fix missing Signed-off-by for included patch (suggested by
	  Baruch)
	- Fix patch naming (suggested by Baruch) 

Changes v1 -> v2:
	- Include patch locally (suggested by Thomas)
---
 ...ehavior-of-recv-in-the-CLOSING-state.patch | 261 ++++++++++++++++++
 1 file changed, 261 insertions(+)
 create mode 100644 package/python-websockets/0001-Fix-behavior-of-recv-in-the-CLOSING-state.patch

Comments

Thomas Petazzoni May 11, 2018, 9:21 p.m. UTC | #1
Hello Joseph,

On Thu, 10 May 2018 15:17:12 -0700, Joseph Kogut wrote:
> Signed-off-by: Joseph Kogut <joseph.kogut@gmail.com>
> 
> Changes v2 -> v3:
> 	- Fix missing Signed-off-by for included patch (suggested by
> 	  Baruch)
> 	- Fix patch naming (suggested by Baruch) 
> 
> Changes v1 -> v2:
> 	- Include patch locally (suggested by Thomas)

The changelog is good, but it should be...

> ---

... below this sign.

Otherwise the changelog is preserved in the commit description when
applying the patch, which is not what we want.

I've fixed that up and applied to master. Thanks!

Thomas
Joseph Kogut May 11, 2018, 9:26 p.m. UTC | #2
Hi Thomas,

On Fri, May 11, 2018 at 2:21 PM, Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
> Hello Joseph,
>
> On Thu, 10 May 2018 15:17:12 -0700, Joseph Kogut wrote:
>> Signed-off-by: Joseph Kogut <joseph.kogut@gmail.com>
>>
>> Changes v2 -> v3:
>>       - Fix missing Signed-off-by for included patch (suggested by
>>         Baruch)
>>       - Fix patch naming (suggested by Baruch)
>>
>> Changes v1 -> v2:
>>       - Include patch locally (suggested by Thomas)
>
> The changelog is good, but it should be...
>
>> ---
>
> ... below this sign.
>

Whoops, I'm not sure how I missed that.

> Otherwise the changelog is preserved in the commit description when
> applying the patch, which is not what we want.
>
> I've fixed that up and applied to master. Thanks!
>

Thanks.

> Thomas
> --
> Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
> Embedded Linux and Kernel engineering
> https://bootlin.com
Peter Korsgaard May 28, 2018, 2:23 p.m. UTC | #3
>>>>> "Joseph" == Joseph Kogut <joseph.kogut@gmail.com> writes:

 > Signed-off-by: Joseph Kogut <joseph.kogut@gmail.com>
 > Changes v2 -> v3:
 > 	- Fix missing Signed-off-by for included patch (suggested by
 > 	  Baruch)
 > 	- Fix patch naming (suggested by Baruch) 

 > Changes v1 -> v2:
 > 	- Include patch locally (suggested by Thomas)

Committed to 2018.02.x, thanks.
diff mbox series

Patch

diff --git a/package/python-websockets/0001-Fix-behavior-of-recv-in-the-CLOSING-state.patch b/package/python-websockets/0001-Fix-behavior-of-recv-in-the-CLOSING-state.patch
new file mode 100644
index 0000000000..cb97b36bde
--- /dev/null
+++ b/package/python-websockets/0001-Fix-behavior-of-recv-in-the-CLOSING-state.patch
@@ -0,0 +1,261 @@ 
+From 402059e4a46a764632eba8a669f5b012f173ee7b Mon Sep 17 00:00:00 2001
+From: Aymeric Augustin <aymeric.augustin@m4x.org>
+Date: Tue, 1 May 2018 17:05:05 +0200
+Subject: [PATCH] Fix behavior of recv() in the CLOSING state.
+
+The behavior wasn't tested correctly: in some test cases, the connection
+had already moved to the CLOSED state, where the close code and reason
+are already known.
+
+Refactor half_close_connection_{local,remote} to allow multiple runs of
+the event loop while remaining in the CLOSING state. Refactor affected
+tests accordingly.
+
+I verified that all tests in the CLOSING state were behaving is intended
+by inserting debug statements in recv/send/ping/pong and running:
+
+$ PYTHONASYNCIODEBUG=1 python -m unittest -v websockets.test_protocol.{Client,Server}Tests.test_{recv,send,ping,pong}_on_closing_connection_{local,remote}
+
+Fix #317, #327, #350, #357.
+
+Signed-off-by: Joseph Kogut <joseph.kogut@gmail.com>
+---
+ websockets/protocol.py      | 10 ++---
+ websockets/test_protocol.py | 78 +++++++++++++++++++++++++++++--------
+ 2 files changed, 66 insertions(+), 22 deletions(-)
+
+diff --git a/websockets/protocol.py b/websockets/protocol.py
+index f8121a1..7583fe9 100644
+--- a/websockets/protocol.py
++++ b/websockets/protocol.py
+@@ -303,7 +303,7 @@ class WebSocketCommonProtocol(asyncio.StreamReaderProtocol):
+         # Don't yield from self.ensure_open() here because messages could be
+         # received before the closing frame even if the connection is closing.
+ 
+-        # Wait for a message until the connection is closed
++        # Wait for a message until the connection is closed.
+         next_message = asyncio_ensure_future(
+             self.messages.get(), loop=self.loop)
+         try:
+@@ -315,15 +315,15 @@ class WebSocketCommonProtocol(asyncio.StreamReaderProtocol):
+             next_message.cancel()
+             raise
+ 
+-        # Now there's no need to yield from self.ensure_open(). Either a
+-        # message was received or the connection was closed.
+-
+         if next_message in done:
+             return next_message.result()
+         else:
+             next_message.cancel()
+             if not self.legacy_recv:
+-                raise ConnectionClosed(self.close_code, self.close_reason)
++                assert self.state in [State.CLOSING, State.CLOSED]
++                # Wait until the connection is closed to raise
++                # ConnectionClosed with the correct code and reason.
++                yield from self.ensure_open()
+ 
+     @asyncio.coroutine
+     def send(self, data):
+diff --git a/websockets/test_protocol.py b/websockets/test_protocol.py
+index 70348fb..bfd4e3b 100644
+--- a/websockets/test_protocol.py
++++ b/websockets/test_protocol.py
+@@ -105,7 +105,7 @@ class CommonTests:
+         self.loop.call_soon(self.loop.stop)
+         self.loop.run_forever()
+ 
+-    def make_drain_slow(self, delay=3 * MS):
++    def make_drain_slow(self, delay=MS):
+         # Process connection_made in order to initialize self.protocol.writer.
+         self.run_loop_once()
+ 
+@@ -174,6 +174,8 @@ class CommonTests:
+         # Empty the outgoing data stream so we can make assertions later on.
+         self.assertOneFrameSent(True, OP_CLOSE, close_frame_data)
+ 
++        assert self.protocol.state is State.CLOSED
++
+     def half_close_connection_local(self, code=1000, reason='close'):
+         """
+         Start a closing handshake but do not complete it.
+@@ -181,31 +183,56 @@ class CommonTests:
+         The main difference with `close_connection` is that the connection is
+         left in the CLOSING state until the event loop runs again.
+ 
++        The current implementation returns a task that must be awaited or
++        cancelled, else asyncio complains about destroying a pending task.
++
+         """
+         close_frame_data = serialize_close(code, reason)
+-        # Trigger the closing handshake from the local side.
+-        self.ensure_future(self.protocol.close(code, reason))
++        # Trigger the closing handshake from the local endpoint.
++        close_task = self.ensure_future(self.protocol.close(code, reason))
+         self.run_loop_once()    # wait_for executes
+         self.run_loop_once()    # write_frame executes
+         # Empty the outgoing data stream so we can make assertions later on.
+         self.assertOneFrameSent(True, OP_CLOSE, close_frame_data)
+-        # Prepare the response to the closing handshake from the remote side.
+-        self.loop.call_soon(
+-            self.receive_frame, Frame(True, OP_CLOSE, close_frame_data))
+-        self.loop.call_soon(self.receive_eof_if_client)
++
++        assert self.protocol.state is State.CLOSING
++
++        # Complete the closing sequence at 1ms intervals so the test can run
++        # at each point even it goes back to the event loop several times.
++        self.loop.call_later(
++            MS, self.receive_frame, Frame(True, OP_CLOSE, close_frame_data))
++        self.loop.call_later(2 * MS, self.receive_eof_if_client)
++
++        # This task must be awaited or cancelled by the caller.
++        return close_task
+ 
+     def half_close_connection_remote(self, code=1000, reason='close'):
+         """
+-        Receive a closing handshake.
++        Receive a closing handshake but do not complete it.
+ 
+         The main difference with `close_connection` is that the connection is
+         left in the CLOSING state until the event loop runs again.
+ 
+         """
++        # On the server side, websockets completes the closing handshake and
++        # closes the TCP connection immediately. Yield to the event loop after
++        # sending the close frame to run the test while the connection is in
++        # the CLOSING state.
++        if not self.protocol.is_client:
++            self.make_drain_slow()
++
+         close_frame_data = serialize_close(code, reason)
+-        # Trigger the closing handshake from the remote side.
++        # Trigger the closing handshake from the remote endpoint.
+         self.receive_frame(Frame(True, OP_CLOSE, close_frame_data))
+-        self.receive_eof_if_client()
++        self.run_loop_once()    # read_frame executes
++        # Empty the outgoing data stream so we can make assertions later on.
++        self.assertOneFrameSent(True, OP_CLOSE, close_frame_data)
++
++        assert self.protocol.state is State.CLOSING
++
++        # Complete the closing sequence at 1ms intervals so the test can run
++        # at each point even it goes back to the event loop several times.
++        self.loop.call_later(2 * MS, self.receive_eof_if_client)
+ 
+     def process_invalid_frames(self):
+         """
+@@ -335,11 +362,13 @@ class CommonTests:
+         self.assertEqual(data, b'tea')
+ 
+     def test_recv_on_closing_connection_local(self):
+-        self.half_close_connection_local()
++        close_task = self.half_close_connection_local()
+ 
+         with self.assertRaises(ConnectionClosed):
+             self.loop.run_until_complete(self.protocol.recv())
+ 
++        self.loop.run_until_complete(close_task)    # cleanup
++
+     def test_recv_on_closing_connection_remote(self):
+         self.half_close_connection_remote()
+ 
+@@ -421,24 +450,29 @@ class CommonTests:
+         self.assertNoFrameSent()
+ 
+     def test_send_on_closing_connection_local(self):
+-        self.half_close_connection_local()
++        close_task = self.half_close_connection_local()
+ 
+         with self.assertRaises(ConnectionClosed):
+             self.loop.run_until_complete(self.protocol.send('foobar'))
++
+         self.assertNoFrameSent()
+ 
++        self.loop.run_until_complete(close_task)    # cleanup
++
+     def test_send_on_closing_connection_remote(self):
+         self.half_close_connection_remote()
+ 
+         with self.assertRaises(ConnectionClosed):
+             self.loop.run_until_complete(self.protocol.send('foobar'))
+-        self.assertOneFrameSent(True, OP_CLOSE, serialize_close(1000, 'close'))
++
++        self.assertNoFrameSent()
+ 
+     def test_send_on_closed_connection(self):
+         self.close_connection()
+ 
+         with self.assertRaises(ConnectionClosed):
+             self.loop.run_until_complete(self.protocol.send('foobar'))
++
+         self.assertNoFrameSent()
+ 
+     # Test the ping coroutine.
+@@ -466,24 +500,29 @@ class CommonTests:
+         self.assertNoFrameSent()
+ 
+     def test_ping_on_closing_connection_local(self):
+-        self.half_close_connection_local()
++        close_task = self.half_close_connection_local()
+ 
+         with self.assertRaises(ConnectionClosed):
+             self.loop.run_until_complete(self.protocol.ping())
++
+         self.assertNoFrameSent()
+ 
++        self.loop.run_until_complete(close_task)    # cleanup
++
+     def test_ping_on_closing_connection_remote(self):
+         self.half_close_connection_remote()
+ 
+         with self.assertRaises(ConnectionClosed):
+             self.loop.run_until_complete(self.protocol.ping())
+-        self.assertOneFrameSent(True, OP_CLOSE, serialize_close(1000, 'close'))
++
++        self.assertNoFrameSent()
+ 
+     def test_ping_on_closed_connection(self):
+         self.close_connection()
+ 
+         with self.assertRaises(ConnectionClosed):
+             self.loop.run_until_complete(self.protocol.ping())
++
+         self.assertNoFrameSent()
+ 
+     # Test the pong coroutine.
+@@ -506,24 +545,29 @@ class CommonTests:
+         self.assertNoFrameSent()
+ 
+     def test_pong_on_closing_connection_local(self):
+-        self.half_close_connection_local()
++        close_task = self.half_close_connection_local()
+ 
+         with self.assertRaises(ConnectionClosed):
+             self.loop.run_until_complete(self.protocol.pong())
++
+         self.assertNoFrameSent()
+ 
++        self.loop.run_until_complete(close_task)    # cleanup
++
+     def test_pong_on_closing_connection_remote(self):
+         self.half_close_connection_remote()
+ 
+         with self.assertRaises(ConnectionClosed):
+             self.loop.run_until_complete(self.protocol.pong())
+-        self.assertOneFrameSent(True, OP_CLOSE, serialize_close(1000, 'close'))
++
++        self.assertNoFrameSent()
+ 
+     def test_pong_on_closed_connection(self):
+         self.close_connection()
+ 
+         with self.assertRaises(ConnectionClosed):
+             self.loop.run_until_complete(self.protocol.pong())
++
+         self.assertNoFrameSent()
+ 
+     # Test the protocol's logic for acknowledging pings with pongs.
+-- 
+2.17.0
+