Merge pull request #47 from scivisum/bug/RD-49887_fix_deadlock

[RD-49887] Fix deadlock.

Author: JonathanWylie

browserdebuggertools/wssessionmanager.py

@@ -4,7 +4,7 @@
 import socket
 import time
 import collections
-from threading import Thread, Lock, Event
+from threading import Thread, Lock, Event, RLock
 
 from typing import Dict, Callable
 
@@ -233,7 +233,7 @@ def __init__(self, port, host, timeout, domains=None):
  self._events_access_lock = Lock()
 
  # Used to manage the health of the message producer
- self._message_producer_lock = Lock() # Lock making sure we don't create 2 ws connections
+ self._message_producer_lock = RLock() # Lock making sure we don't create 2 ws connections
  self._last_not_ok = None
  self._message_producer_not_ok_count = 0
  self._send_queue = collections.deque()
@@ -255,6 +255,9 @@ def _check_message_producer(self):
  try:
  self._message_producer.health_check()
  except (websocket.WebSocketConnectionClosedException, WebSocketBlockedException):
+ # If the current ws connection is only blocked, we better make sure we close it
+ # before opening a new one.
+ self._message_producer.close()
  self._increment_message_producer_not_ok()
  self._setup_ws_session()
 

setup.py

@@ -12,7 +12,7 @@
 
 setup(
  name="browserdebuggertools",
- version="6.1.0",
+ version="6.1.1",
  python_requires='>=3.8',
  packages=PACKAGES,
  install_requires=requires,

tests/integrationtests/test_wssessionmanager.py

@@ -53,7 +53,7 @@ def send(self, data):
 
 
 class Test_WSSessionManager__execute(TestCase):
- """ It's very hard to make this test fail but it will catch major regressions
+ """ It's very hard to make this test fail, but it will catch major regressions
  """
  def setUp(self):
  self.pool = ThreadPool(10)
@@ -84,7 +84,7 @@ def _send(message):
 
 
 class Test_WSSessionManager_get_events(TestCase):
- """ It's very hard to make this test fail but it will catch major regressions
+ """ It's very hard to make this test fail, but it will catch major regressions
  """
 
  def test_locked_get_events(self):
@@ -188,11 +188,36 @@ def close(self):
  pass
 
 
+class Test_WSSessionManager_blocked_with_domains(TestCase):
+ """ We need this test to prove we have fixed the problem of a message producer being blocked.
+ When it tried to recreate the websocket, it shouldn't deadlock.
+ """
+
+ def tearDown(self):
+ self.session_manager.close()
+
+ def test_deadlock(self):
+
+ with patch.object(
+ _WSMessageProducer, "_get_websocket",
+ new=MagicMock(return_value=_DummyWebsocket())
+ ):
+
+ self.session_manager = WSSessionManager(
+ 1234, "localhost", 2, domains={"Network": {}}
+ )
+ # The first message producer and web socket are clocked.
+ # When the ws session is recreated we get a new one which won't be blocked.
+ self.session_manager._message_producer._BLOCKED_TIMEOUT = -1
+
+ self.session_manager.execute("Network", "enable")
+
+
 class Test_WSSessionManager_execute(TestCase):
 
  @staticmethod
  def resetWS():
- """ flush_messages runs async so we only want the socket to start blocking when we're
+ """ flush_messages runs async, so we only want the socket to start blocking when we're
  ready
  """
  ExceptionThrowingWS.exceptions = 0
@@ -218,6 +243,7 @@ def test_thread_blocked_once(self):
  start = time.time()
  self.session_manager.execute("Network", "enable")
 
+ # todo wtf
  # We should find the execution result after 5 seconds because
  # we give the thread 5 seconds to poll before we consider it blocked,
  self.assertLess(time.time() - start, 10)
@@ -241,8 +267,8 @@ def test_thread_blocks_causes_timeout(self):
 
  self.session_manager = WSSessionManager(1234, "localhost", 3)
  self.resetWS()
+ start = time.time()
  with self.assertRaises(DevToolsTimeoutException):
- start = time.time()
  self.session_manager.execute("Network", "enable")
  self.assertLess(time.time() - start, 5)