Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hang when exception is raised in _quit_loop_by_signal #114

Closed
The-Compiler opened this issue Dec 16, 2015 · 4 comments
Closed

Hang when exception is raised in _quit_loop_by_signal #114

The-Compiler opened this issue Dec 16, 2015 · 4 comments

Comments

@The-Compiler
Copy link
Member

I haven't investigated that closely yet, thought I'd open an issue before I forget.

When raising an exception in _quit_loop_by_signal:

diff --git a/pytestqt/wait_signal.py b/pytestqt/wait_signal.py
index c32a76c..7efc019 100644
--- a/pytestqt/wait_signal.py
+++ b/pytestqt/wait_signal.py
@@ -111,6 +111,7 @@ class SignalBlocker(_AbstractSignalBlocker):
         """
         quits the event loop and marks that we finished because of a signal.
         """
+        raise Exception
         self.signal_triggered = True
         self._loop.quit()
         self._cleanup()

Then pytest-qt's tests seem to hang:

$ tox -e py35-pyqt5 -- tests/test_wait_signal.py -v 
[...]
test_wait_signal.py::test_signal_blocker_exception PASSED
test_wait_signal.py::test_signal_triggered[200-None-True-wait_function0-True] 
@The-Compiler
Copy link
Member Author

I think this is just happening because self._loop.quit() is never called, so we're stuck in the inner event loop, and handling the exception (which happened inside a virtual Qt function) will never be done.

I'm not sure there's anything good we can do about this (other than "don't raise an exception there" 😆), so if you can't think of anything either, I'm fine with closing this.

@nicoddemus
Copy link
Member

Perhaps we can make the code more robust to errors by wrapping it into a try/finally:

         """
         quits the event loop and marks that we finished because of a signal.
         """
         try:
             self.signal_triggered = True
             self._cleanup()
         finally:
             self._loop.quit()

@The-Compiler
Copy link
Member Author

I thought about that too and thought it wasn't necessary because I can't imagine there was a condition where self.signal_triggered = True raises.

That being said, I think it's good to have it there as a "reminder" so any future additions (which could maybe raise) are in the try block, so 👍.

@nicoddemus
Copy link
Member

I think it's good to have it there as a "reminder" so any future additions

Yeah, that's exactly the intent, less for the current code and more as a reminder for future additions. Sorry for not being explicit. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants