From d19d1ef62c1b91ede1ab949837526fd6c8758e35 Mon Sep 17 00:00:00 2001 From: Philip Scadding Date: Thu, 11 Jun 2020 11:27:01 +0100 Subject: [PATCH 1/7] Fixes an error that gets thrown when leaving a project on OSX. It seems it errors if the connection gets closed mid loop. --- python/tk_desktop/rpc.py | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/python/tk_desktop/rpc.py b/python/tk_desktop/rpc.py index b8216a43..35ca1230 100644 --- a/python/tk_desktop/rpc.py +++ b/python/tk_desktop/rpc.py @@ -256,7 +256,7 @@ def wrapper(*args, **kwargs): def run(self): """ Run the thread, accepting connections and then listening on them until - they are closed. Each message is a call into the function table. + they are closed. Each message is a call into the function table. """ logger.debug("server listening on '%s'", self.pipe) while self._is_closed is False: @@ -278,10 +278,18 @@ def run(self): raise ready = False else: - # can use select on osx and linux - (rd, _, _) = select.select( - [self.server._listener._socket], [], [], self.LISTEN_TIMEOUT - ) + # Can use select on OSX and Linux. + try: + logger.info("_socket %s" % self.server._listener._socket) + (rd, _, _) = select.select( + [self.server._listener._socket], [], [], self.LISTEN_TIMEOUT + ) + except select.error as e: + if "Bad file descriptor" in e.args: + # The socket was likely closed during this iteration of the loop + # so we should break out of the loop. + break + raise ready = len(rd) > 0 if not ready: From 33f82a85ca196137cbf0ead648960e3e52056fb0 Mon Sep 17 00:00:00 2001 From: Philip Scadding Date: Thu, 11 Jun 2020 11:33:01 +0100 Subject: [PATCH 2/7] Removed temporary logging line. --- python/tk_desktop/rpc.py | 1 - 1 file changed, 1 deletion(-) diff --git a/python/tk_desktop/rpc.py b/python/tk_desktop/rpc.py index 35ca1230..c2f2a721 100644 --- a/python/tk_desktop/rpc.py +++ b/python/tk_desktop/rpc.py @@ -280,7 +280,6 @@ def run(self): else: # Can use select on OSX and Linux. try: - logger.info("_socket %s" % self.server._listener._socket) (rd, _, _) = select.select( [self.server._listener._socket], [], [], self.LISTEN_TIMEOUT ) From 7467179baaeabb2ad309703ad798c2a1794c13ba Mon Sep 17 00:00:00 2001 From: Philip Scadding Date: Thu, 11 Jun 2020 15:40:49 +0100 Subject: [PATCH 3/7] Alters rpc test so that any errors raised in the server are reported. This will currently mean that the tests fail, but I need to fix the cause of the failure. --- tests/test_rpc.py | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/tests/test_rpc.py b/tests/test_rpc.py index 39c6fc0a..391e6a8c 100644 --- a/tests/test_rpc.py +++ b/tests/test_rpc.py @@ -112,6 +112,25 @@ def long_call(self, close_proxy, return_value=None): return return_value +class CaptureErrorServer(RPCServerThread): + """ + This is the RPCServerThread but it captures any errors + raised during the run and re-raises them in the join so that the tests fail. + """ + + def run(self): + self.exc = None + try: + super(CaptureErrorServer, self).run() + except BaseException as e: + self.exc = e + + def join(self): + super(CaptureErrorServer, self).join() + if self.exc: + raise self.exc + + @pytest.fixture def fake_engine(): """ @@ -128,7 +147,7 @@ def server(fake_engine, request): :returns: A RPCServerThread instance. """ - server = RPCServerThread(fake_engine) + server = CaptureErrorServer(fake_engine) server.register_function(fake_engine.pass_arg) server.register_function(fake_engine.pass_named_arg) server.register_function(fake_engine.boom) From f9883ed2cb77325be354124067b649c61e043fcb Mon Sep 17 00:00:00 2001 From: Philip Scadding Date: Mon, 15 Jun 2020 11:36:15 +0100 Subject: [PATCH 4/7] Commented out the raising of the exception but left the class so it can be reimplemented when we are ready to fix it. --- tests/test_rpc.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/test_rpc.py b/tests/test_rpc.py index 391e6a8c..09110896 100644 --- a/tests/test_rpc.py +++ b/tests/test_rpc.py @@ -116,6 +116,7 @@ class CaptureErrorServer(RPCServerThread): """ This is the RPCServerThread but it captures any errors raised during the run and re-raises them in the join so that the tests fail. + The idea was taken from here: https://stackoverflow.com/a/31614591/4223964 """ def run(self): @@ -127,8 +128,13 @@ def run(self): def join(self): super(CaptureErrorServer, self).join() - if self.exc: - raise self.exc + # TODO: This is raising more errors than we want at the time writing + # so it is commented out. + # We should revisit it and address the errors at some point but they + # are not affecting functionality at the moment nor are they that + # visible to the user. + # if self.exc: + # raise self.exc @pytest.fixture From e480d7c018ba809a590268a7c1d807360284d6d2 Mon Sep 17 00:00:00 2001 From: Philip Scadding Date: Tue, 16 Jun 2020 14:25:14 +0100 Subject: [PATCH 5/7] Removes the wrapper class that reraises server errors in the tests. --- python/tk_desktop/rpc.py | 7 +++++++ tests/test_rpc.py | 32 ++++++-------------------------- 2 files changed, 13 insertions(+), 26 deletions(-) diff --git a/python/tk_desktop/rpc.py b/python/tk_desktop/rpc.py index c2f2a721..87020073 100644 --- a/python/tk_desktop/rpc.py +++ b/python/tk_desktop/rpc.py @@ -194,6 +194,13 @@ class RPCServerThread(threading.Thread): list/dictionary are treated as args/kwargs for the function call. """ + # FIXME: Any errors raised here when closing won't actually be caught by + # our tests, because the error happens in a separate thread. This can be solved + # by catching and storing the exception and then raising it in the join method + # as suggested here: https://stackoverflow.com/a/31614591/4223964 + # This is currently not implemented because it raises errors in our tests + # that we at the time of writing don't have time to fix, but have no impact on user experience. + # timeout in seconds to wait for a request LISTEN_TIMEOUT = 2 diff --git a/tests/test_rpc.py b/tests/test_rpc.py index 09110896..c531d03f 100644 --- a/tests/test_rpc.py +++ b/tests/test_rpc.py @@ -112,31 +112,6 @@ def long_call(self, close_proxy, return_value=None): return return_value -class CaptureErrorServer(RPCServerThread): - """ - This is the RPCServerThread but it captures any errors - raised during the run and re-raises them in the join so that the tests fail. - The idea was taken from here: https://stackoverflow.com/a/31614591/4223964 - """ - - def run(self): - self.exc = None - try: - super(CaptureErrorServer, self).run() - except BaseException as e: - self.exc = e - - def join(self): - super(CaptureErrorServer, self).join() - # TODO: This is raising more errors than we want at the time writing - # so it is commented out. - # We should revisit it and address the errors at some point but they - # are not affecting functionality at the moment nor are they that - # visible to the user. - # if self.exc: - # raise self.exc - - @pytest.fixture def fake_engine(): """ @@ -153,7 +128,12 @@ def server(fake_engine, request): :returns: A RPCServerThread instance. """ - server = CaptureErrorServer(fake_engine) + + # FIXME: Any errors raised here when closing won't actually be caught by + # our tests, because the error happens in a separate thread. + # See the FIX ME in the RPCServerThread class for further details. + + server = RPCServerThread(fake_engine) server.register_function(fake_engine.pass_arg) server.register_function(fake_engine.pass_named_arg) server.register_function(fake_engine.boom) From 048fe06c814da0950dd5fb6bbed601179ae3d833 Mon Sep 17 00:00:00 2001 From: Philip Scadding Date: Wed, 17 Jun 2020 10:57:01 +0100 Subject: [PATCH 6/7] Takes a different approach to catching the error during close. --- python/tk_desktop/rpc.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/python/tk_desktop/rpc.py b/python/tk_desktop/rpc.py index 87020073..924eac5a 100644 --- a/python/tk_desktop/rpc.py +++ b/python/tk_desktop/rpc.py @@ -290,10 +290,14 @@ def run(self): (rd, _, _) = select.select( [self.server._listener._socket], [], [], self.LISTEN_TIMEOUT ) - except select.error as e: - if "Bad file descriptor" in e.args: - # The socket was likely closed during this iteration of the loop - # so we should break out of the loop. + except Exception: + # It is possible that the server is in the process of being closed. + # In which case it can throw any number of different errors depending on + # where in the process it closed. If we wait a short while we can then check + # to see if the server was actually closed and just back out gracefully + # rather than reporting the error. + time.sleep(1) + if self._is_closed: break raise ready = len(rd) > 0 From 44cd19662026b585c14f2ee9213dc3cca03779b8 Mon Sep 17 00:00:00 2001 From: Philip Scadding Date: Wed, 17 Jun 2020 16:48:47 +0100 Subject: [PATCH 7/7] Revert "Takes a different approach to catching the error during close." This reverts commit 048fe06c814da0950dd5fb6bbed601179ae3d833. --- python/tk_desktop/rpc.py | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/python/tk_desktop/rpc.py b/python/tk_desktop/rpc.py index 924eac5a..87020073 100644 --- a/python/tk_desktop/rpc.py +++ b/python/tk_desktop/rpc.py @@ -290,14 +290,10 @@ def run(self): (rd, _, _) = select.select( [self.server._listener._socket], [], [], self.LISTEN_TIMEOUT ) - except Exception: - # It is possible that the server is in the process of being closed. - # In which case it can throw any number of different errors depending on - # where in the process it closed. If we wait a short while we can then check - # to see if the server was actually closed and just back out gracefully - # rather than reporting the error. - time.sleep(1) - if self._is_closed: + except select.error as e: + if "Bad file descriptor" in e.args: + # The socket was likely closed during this iteration of the loop + # so we should break out of the loop. break raise ready = len(rd) > 0