From 0ebab5edaa9511a5cc5730d2650de34f64a758b0 Mon Sep 17 00:00:00 2001 From: Zsin Skri Date: Mon, 15 Jul 2019 21:25:39 +0200 Subject: [PATCH 1/7] fix "Sporadic test failures in BPD tests #3309" The bpd test bind a socket in order to test the protocol implementation. When running concurrently this often resulted in an attempt to bind an already occupied port. By using the port number `0` we instead let the OS choose a free port. We then have to extract it from the socket (which is handled by `bluelet`) via `mock.patch`ing. --- test/test_player.py | 68 ++++++++++++++++++++++++++------------------- 1 file changed, 40 insertions(+), 28 deletions(-) diff --git a/test/test_player.py b/test/test_player.py index 959d77eb3..036593bff 100644 --- a/test/test_player.py +++ b/test/test_player.py @@ -29,9 +29,8 @@ import time import yaml import tempfile from contextlib import contextmanager -import random -from beets.util import py3_path +from beets.util import py3_path, bluelet from beetsplug import bpd import confuse @@ -231,11 +230,6 @@ class MPCClient(object): return line -def start_beets(*args): - import beets.ui - beets.ui.main(list(args)) - - def implements(commands, expectedFailure=False): # noqa: N803 def _test(self): with self.run_bpd() as client: @@ -263,22 +257,18 @@ class BPDTestHelper(unittest.TestCase, TestHelper): self.unload_plugins() @contextmanager - def run_bpd(self, host='localhost', port=None, password=None, - do_hello=True, second_client=False): + def run_bpd(self, host='localhost', password=None, do_hello=True, + second_client=False): """ Runs BPD in another process, configured with the same library database as we created in the setUp method. Exposes a client that is connected to the server, and kills the server at the end. """ - # Choose a port (randomly) to avoid conflicts between parallel - # tests. - if not port: - port = 9876 + random.randint(0, 10000) - # Create a config file: config = { 'pluginpath': [py3_path(self.temp_dir)], 'plugins': 'bpd', - 'bpd': {'host': host, 'port': port, 'control_port': port + 1}, + # use port 0 to let the OS choose a free port + 'bpd': {'host': host, 'port': 0, 'control_port': 0}, } if password: config['bpd']['password'] = password @@ -289,28 +279,50 @@ class BPDTestHelper(unittest.TestCase, TestHelper): yaml.dump(config, Dumper=confuse.Dumper, encoding='utf-8')) config_file.close() + bluelet_listener = bluelet.Listener + @mock.patch("beets.util.bluelet.Listener") + def start_server(assigned_port, listener_patch): + """Start the bpd server, writing the port to `assigned_port`. + """ + def listener_wrap(host, port): + """Wrap `bluelet.Listener`, writing the port to `assigend_port`. + + `bluelet.Listener` has previously been saved to + `bluelet_listener` as this function will replace it at its + original location. + """ + listener = bluelet_listener(host, port) + assigned_port.value = listener.sock.getsockname()[1] + return listener + listener_patch.side_effect = listener_wrap + + import beets.ui + beets.ui.main([ + '--library', self.config['library'].as_filename(), + '--directory', py3_path(self.libdir), + '--config', py3_path(config_file.name), + 'bpd' + ]) + # Fork and launch BPD in the new process: - args = ( - '--library', self.config['library'].as_filename(), - '--directory', py3_path(self.libdir), - '--config', py3_path(config_file.name), - 'bpd' - ) - server = mp.Process(target=start_beets, args=args) + assigned_port = mp.Value("I", 0) + server = mp.Process(target=start_server, args=(assigned_port,)) server.start() # Wait until the socket is connected: - sock, sock2 = None, None for _ in range(20): - sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) - if sock.connect_ex((host, port)) == 0: + if assigned_port.value != 0: + # read which port has been assigned by the OS + port = assigned_port.value break - else: - sock.close() - time.sleep(0.01) + time.sleep(0.01) else: raise RuntimeError('Timed out waiting for the BPD server') + sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + sock.connect((host, port)) + + sock2 = None try: if second_client: sock2 = socket.socket(socket.AF_INET, socket.SOCK_STREAM) From bbda292145d3c9758a4e7a849d822b3283e3b524 Mon Sep 17 00:00:00 2001 From: Zsin Skri Date: Mon, 15 Jul 2019 22:25:48 +0200 Subject: [PATCH 2/7] bpd test: make `start_server` a freestanding function Under some circumstances (maybe under MS Windows?) local objects can't be pickled. When `start_server` is a local this causes a crash: https://ci.appveyor.com/project/beetbox/beets/builds/25996163/job/rbp3frnkwsvbuwx6#L541 Make `start_server` a freestanding function to mitigate this. --- test/test_player.py | 53 +++++++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 26 deletions(-) diff --git a/test/test_player.py b/test/test_player.py index 036593bff..37d0b6640 100644 --- a/test/test_player.py +++ b/test/test_player.py @@ -240,6 +240,27 @@ def implements(commands, expectedFailure=False): # noqa: N803 return unittest.expectedFailure(_test) if expectedFailure else _test +bluelet_listener = bluelet.Listener +@mock.patch("beets.util.bluelet.Listener") +def start_server(args, assigned_port, listener_patch): + """Start the bpd server, writing the port to `assigned_port`. + """ + def listener_wrap(host, port): + """Wrap `bluelet.Listener`, writing the port to `assigend_port`. + + `bluelet.Listener` has previously been saved to + `bluelet_listener` as this function will replace it at its + original location. + """ + listener = bluelet_listener(host, port) + assigned_port.value = listener.sock.getsockname()[1] + return listener + listener_patch.side_effect = listener_wrap + + import beets.ui + beets.ui.main(args) + + class BPDTestHelper(unittest.TestCase, TestHelper): def setUp(self): self.setup_beets(disk=True) @@ -279,34 +300,14 @@ class BPDTestHelper(unittest.TestCase, TestHelper): yaml.dump(config, Dumper=confuse.Dumper, encoding='utf-8')) config_file.close() - bluelet_listener = bluelet.Listener - @mock.patch("beets.util.bluelet.Listener") - def start_server(assigned_port, listener_patch): - """Start the bpd server, writing the port to `assigned_port`. - """ - def listener_wrap(host, port): - """Wrap `bluelet.Listener`, writing the port to `assigend_port`. - - `bluelet.Listener` has previously been saved to - `bluelet_listener` as this function will replace it at its - original location. - """ - listener = bluelet_listener(host, port) - assigned_port.value = listener.sock.getsockname()[1] - return listener - listener_patch.side_effect = listener_wrap - - import beets.ui - beets.ui.main([ - '--library', self.config['library'].as_filename(), - '--directory', py3_path(self.libdir), - '--config', py3_path(config_file.name), - 'bpd' - ]) - # Fork and launch BPD in the new process: assigned_port = mp.Value("I", 0) - server = mp.Process(target=start_server, args=(assigned_port,)) + server = mp.Process(target=start_server, args=([ + '--library', self.config['library'].as_filename(), + '--directory', py3_path(self.libdir), + '--config', py3_path(config_file.name), + 'bpd' + ], assigned_port)) server.start() # Wait until the socket is connected: From fb07a5112a8284a533ffb8ec1242d5e23b3eafd4 Mon Sep 17 00:00:00 2001 From: Zsin Skri Date: Wed, 17 Jul 2019 13:16:01 +0200 Subject: [PATCH 3/7] bpd tests: terminate server upon connection failure --- test/test_player.py | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/test/test_player.py b/test/test_player.py index 37d0b6640..77ebef7d4 100644 --- a/test/test_player.py +++ b/test/test_player.py @@ -310,21 +310,23 @@ class BPDTestHelper(unittest.TestCase, TestHelper): ], assigned_port)) server.start() - # Wait until the socket is connected: - for _ in range(20): - if assigned_port.value != 0: - # read which port has been assigned by the OS - port = assigned_port.value - break - time.sleep(0.01) - else: - raise RuntimeError('Timed out waiting for the BPD server') - - sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) - sock.connect((host, port)) - - sock2 = None try: + # Wait until the socket is connected: + for _ in range(20): + if assigned_port.value != 0: + # read which port has been assigned by the OS + port = assigned_port.value + break + time.sleep(0.01) + else: + raise RuntimeError( + 'Timed out waiting for the BPD server to start' + ) + + sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + sock.connect((host, port)) + + sock2 = None if second_client: sock2 = socket.socket(socket.AF_INET, socket.SOCK_STREAM) sock2.connect((host, port)) From 871f79c8f2dee43b313142ed9ddc2a414252fe9a Mon Sep 17 00:00:00 2001 From: Zsin Skri Date: Wed, 17 Jul 2019 13:38:57 +0200 Subject: [PATCH 4/7] bpd tests: close only existing sockets Close sockets in `finally`-clauses only after they have actually been created. --- test/test_player.py | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/test/test_player.py b/test/test_player.py index 77ebef7d4..b9d35281b 100644 --- a/test/test_player.py +++ b/test/test_player.py @@ -324,19 +324,25 @@ class BPDTestHelper(unittest.TestCase, TestHelper): ) sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) - sock.connect((host, port)) + try: + sock.connect((host, port)) - sock2 = None - if second_client: - sock2 = socket.socket(socket.AF_INET, socket.SOCK_STREAM) - sock2.connect((host, port)) - yield MPCClient(sock, do_hello), MPCClient(sock2, do_hello) - else: - yield MPCClient(sock, do_hello) + if second_client: + sock2 = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + try: + sock2.connect((host, port)) + yield ( + MPCClient(sock, do_hello), + MPCClient(sock2, do_hello), + ) + finally: + sock2.close() + + else: + yield MPCClient(sock, do_hello) + finally: + sock.close() finally: - sock.close() - if sock2: - sock2.close() server.terminate() server.join(timeout=0.2) From 8ba79117d936870aff39c42a909348d7f254d7cd Mon Sep 17 00:00:00 2001 From: Zsin Skri Date: Wed, 17 Jul 2019 16:08:46 +0200 Subject: [PATCH 5/7] bpd tests: use mp.Queue to communicate assigned port Use a `multiprocessing.Queue` instead of a `multiprocessing.Value` to avoid the manual polling/timeout handling. TODO: Strangely Listener seems to be constructed twice. Only the second one is used. Fix that and then remove the code working around it. --- test/test_player.py | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/test/test_player.py b/test/test_player.py index b9d35281b..78e3948ca 100644 --- a/test/test_player.py +++ b/test/test_player.py @@ -247,13 +247,14 @@ def start_server(args, assigned_port, listener_patch): """ def listener_wrap(host, port): """Wrap `bluelet.Listener`, writing the port to `assigend_port`. - - `bluelet.Listener` has previously been saved to - `bluelet_listener` as this function will replace it at its - original location. """ + # `bluelet.Listener` has previously been saved to + # `bluelet_listener` as this function will replace it at its + # original location. listener = bluelet_listener(host, port) - assigned_port.value = listener.sock.getsockname()[1] + # read which port has been assigned by the OS + # TODO: change to put_nowait. There should always be a free slot. + assigned_port.put(listener.sock.getsockname()[1]) return listener listener_patch.side_effect = listener_wrap @@ -301,7 +302,7 @@ class BPDTestHelper(unittest.TestCase, TestHelper): config_file.close() # Fork and launch BPD in the new process: - assigned_port = mp.Value("I", 0) + assigned_port = mp.Queue(1) server = mp.Process(target=start_server, args=([ '--library', self.config['library'].as_filename(), '--directory', py3_path(self.libdir), @@ -311,17 +312,12 @@ class BPDTestHelper(unittest.TestCase, TestHelper): server.start() try: - # Wait until the socket is connected: - for _ in range(20): - if assigned_port.value != 0: - # read which port has been assigned by the OS - port = assigned_port.value - break - time.sleep(0.01) - else: - raise RuntimeError( - 'Timed out waiting for the BPD server to start' - ) + # TODO: ugly hack. remove + print("ignoring port in queue:", assigned_port.get(timeout=2)) + # Wait until the socket is connected + port = assigned_port.get(timeout=2) + print("test bpd server on port", port) + time.sleep(0.1) sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) try: From 088af4d1714032e69956918048cf30f4be7ddc4b Mon Sep 17 00:00:00 2001 From: Zsin Skri Date: Wed, 17 Jul 2019 19:03:29 +0200 Subject: [PATCH 6/7] bpd tests: skip `control_port` in queue When setting up bpd tests, two servers are startet: first a control server, then bpd. Both send their assigned ports down a queue. The recipient only needs bpd's port and thus skips the first queue entry. --- test/test_player.py | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/test/test_player.py b/test/test_player.py index 78e3948ca..021a04fc0 100644 --- a/test/test_player.py +++ b/test/test_player.py @@ -252,9 +252,8 @@ def start_server(args, assigned_port, listener_patch): # `bluelet_listener` as this function will replace it at its # original location. listener = bluelet_listener(host, port) - # read which port has been assigned by the OS - # TODO: change to put_nowait. There should always be a free slot. - assigned_port.put(listener.sock.getsockname()[1]) + # read port assigned by OS + assigned_port.put_nowait(listener.sock.getsockname()[1]) return listener listener_patch.side_effect = listener_wrap @@ -302,7 +301,7 @@ class BPDTestHelper(unittest.TestCase, TestHelper): config_file.close() # Fork and launch BPD in the new process: - assigned_port = mp.Queue(1) + assigned_port = mp.Queue(2) # 2 slots, `control_port` and `port` server = mp.Process(target=start_server, args=([ '--library', self.config['library'].as_filename(), '--directory', py3_path(self.libdir), @@ -312,12 +311,8 @@ class BPDTestHelper(unittest.TestCase, TestHelper): server.start() try: - # TODO: ugly hack. remove - print("ignoring port in queue:", assigned_port.get(timeout=2)) - # Wait until the socket is connected - port = assigned_port.get(timeout=2) - print("test bpd server on port", port) - time.sleep(0.1) + assigned_port.get(timeout=1) # skip control_port + port = assigned_port.get(timeout=0.5) # read port sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) try: From 5e5cb3cd4349d31ff5ab429ab723b517cf1844aa Mon Sep 17 00:00:00 2001 From: Zsin Skri Date: Wed, 17 Jul 2019 19:26:45 +0200 Subject: [PATCH 7/7] changelog: fix "Sporadic test failures in BPD tests #3309" #3330 Add a changelog entry asking plugin developers to report any further occurrences of this failure. --- docs/changelog.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index aa544bcac..001509272 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -80,6 +80,9 @@ For plugin developers: value. Thanks to :user:`zsinskri`. :bug:`3329` +* There were sporadic failures in ``test.test_player``. Hopefully these are + fixed. If they resurface, please reopen the relevant issue. + :bug:`3309` :bug:`3330` For packagers: