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

Rework ui_dispatch tests to avoid need for Qt #1792

Merged
merged 2 commits into from
May 7, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions traits/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@
)

from .trait_notifiers import (
get_ui_handler,
set_ui_handler,
push_exception_handler,
pop_exception_handler,
TraitChangeNotifyWrapper,
Expand Down
5 changes: 5 additions & 0 deletions traits/api.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -178,3 +178,8 @@ from .trait_numeric import (
ArrayOrNone as ArrayOrNone,
CArray as CArray,
)

from .trait_notifiers import (
get_ui_handler as get_ui_handler,
set_ui_handler as set_ui_handler,
)
8 changes: 4 additions & 4 deletions traits/tests/test_new_notifiers.py
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,8 @@ def test_notification_on_separate_thread(self):
receiver = Receiver()

def on_foo_notifications(obj, name, old, new):
thread_id = threading.current_thread().ident
event = (thread_id, obj, name, old, new)
thread = threading.current_thread()
event = (thread, obj, name, old, new)
receiver.notifications.append(event)

obj = Foo()
Expand All @@ -97,5 +97,5 @@ def on_foo_notifications(obj, name, old, new):
self.assertEqual(len(notifications), 1)
self.assertEqual(notifications[0][1:], (obj, "foo", 0, 3))

this_thread_id = threading.current_thread().ident
self.assertNotEqual(this_thread_id, notifications[0][0])
this_thread = threading.current_thread()
self.assertNotEqual(this_thread, notifications[0][0])
195 changes: 129 additions & 66 deletions traits/tests/test_ui_notifiers.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,33 +18,22 @@
notification really occurs on the UI thread.

At present, `dispatch='ui'` and `dispatch='fast_ui'` have the same effect.

"""

import asyncio
import contextlib
import threading
import time
import unittest

# Preamble: Try importing Qt, and set QT_FOUND to True on success.
try:
from pyface.util.guisupport import get_app_qt4

# This import is necessary to set the `ui_handler` global variable in
# `traits.trait_notifiers`, which is responsible for dispatching the events
# to the UI thread.
from traitsui.qt import toolkit # noqa: F401

qt4_app = get_app_qt4()

except Exception:
QT_FOUND = False

else:
QT_FOUND = True


from traits.api import (
Callable,
Float,
get_ui_handler,
HasTraits,
on_trait_change,
set_ui_handler,
)
from traits import trait_notifiers
from traits.api import Callable, Float, HasTraits, on_trait_change


class CalledAsMethod(HasTraits):
Expand All @@ -61,88 +50,162 @@ def on_foo_change(self, obj, name, old, new):
self.callback(obj, name, old, new)


class BaseTestUINotifiers(object):
""" Tests for dynamic notifiers with `dispatch='ui'`.
def asyncio_ui_handler(event_loop):
"""
Create a UI handler that dispatches to the asyncio event loop.
"""

def ui_handler(handler, *args, **kwargs):
"""
UI handler that dispatches to the asyncio event loop.
"""
event_loop.call_soon_threadsafe(lambda: handler(*args, **kwargs))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat!


return ui_handler


@contextlib.contextmanager
def use_asyncio_ui_handler(event_loop):
"""
Context manager that temporarily sets the UI handler to an asyncio handler.
"""
old_handler = get_ui_handler()
set_ui_handler(asyncio_ui_handler(event_loop))
try:
yield
finally:
set_ui_handler(old_handler)


@contextlib.contextmanager
def clear_ui_handler():
"""
Context manager that temporarily clears the UI handler.
"""
old_handler = get_ui_handler()
set_ui_handler(None)
try:
yield
finally:
set_ui_handler(old_handler)


class BaseTestUINotifiers(object):
"""Tests for dynamic notifiers with `dispatch='ui'`."""

#### 'TestCase' protocol ##################################################

def setUp(self):
self.notifications = []
self.exceptions = []
self.done = asyncio.Event()
self.obj = self.obj_factory()
trait_notifiers.push_exception_handler(self.handle_exception)
self.addCleanup(trait_notifiers.pop_exception_handler)

def enterContext(self, cm):
# Backport of Python 3.11's TestCase.enterContext method.
result = type(cm).__enter__(cm)
self.addCleanup(type(cm).__exit__, cm, None, None, None)
return result

#### 'TestUINotifiers' protocol ###########################################

def flush_event_loop(self):
""" Post and process the Qt events. """
qt4_app.sendPostedEvents()
qt4_app.processEvents()
def handle_exception(self, object, name, old, new):
self.exceptions.append((object, name, old, new))

def modify_obj(self):
trait_notifiers.push_exception_handler(self.handle_exception)
try:
self.obj.foo = 3
finally:
trait_notifiers.pop_exception_handler()

def on_foo_notifications(self, obj, name, old, new):
thread_id = threading.current_thread().ident
event = (thread_id, (obj, name, old, new))
event = (threading.current_thread(), (obj, name, old, new))
self.notifications.append(event)
self.done.set()

#### Tests ################################################################

@unittest.skipIf(
not QT_FOUND, "Qt event loop not found, UI dispatch not possible."
)
def test_notification_from_main_thread(self):

obj = self.obj_factory()
def test_notification_from_main_thread_with_no_ui_handler(self):
# Given
self.enterContext(clear_ui_handler())

obj.foo = 3
self.flush_event_loop()
# When we set obj.foo to 3 on the main thread.
self.modify_obj()

notifications = self.notifications
self.assertEqual(len(notifications), 1)
# Then the notification is processed synchronously on the main thread.
self.assertEqual(
self.notifications,
[(threading.main_thread(), (self.obj, "foo", 0, 3))],
)

thread_id, event = notifications[0]
self.assertEqual(event, (obj, "foo", 0, 3))
def test_notification_from_main_thread_with_registered_ui_handler(self):
# Given
self.enterContext(use_asyncio_ui_handler(asyncio.get_event_loop()))

ui_thread = trait_notifiers.ui_thread
self.assertEqual(thread_id, ui_thread)
# When we set obj.foo to 3 on the main thread.
self.modify_obj()

@unittest.skipIf(
not QT_FOUND, "Qt event loop not found, UI dispatch not possible."
)
def test_notification_from_separate_thread(self):
# Then the notification is processed synchronously on the main thread.
self.assertEqual(
self.notifications,
[(threading.main_thread(), (self.obj, "foo", 0, 3))],
)

obj = self.obj_factory()
def test_notification_from_separate_thread_failure_case(self):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I understand the context for this test case, but just to be clear: do we accept that any background process unknown to the UI will be able to modify objects, but ideally we also want to know about it?

If so, is this a new edge case that we can just test more easily without Qt, or does this happen often?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the key point of the test (and of the dispatch="ui" functionality) is that even though the object is modified off the main thread, the trait change handlers are run in the main UI thread.

That said, this pattern isn't ideal in the first place; my preference is to use something like Traits Futures so that we're not modifying the same object from multiple threads in the first place.

Not sure whether this answers your question or not ... ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see - so the main result of this test is that we don't see any notifiers and we observe an exception, since the handlers are always on the main UI tread.

That helps understand the context, yes, thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right - the notifiers don't get executed at all in this case.

# Given no registered ui handler
self.enterContext(clear_ui_handler())

# Set obj.foo to 3 on a separate thread.
def set_foo_to_3(obj):
obj.foo = 3
# When we set obj.foo to 3 on a separate thread.
background_thread = threading.Thread(target=self.modify_obj)
background_thread.start()
self.addCleanup(background_thread.join)

threading.Thread(target=set_foo_to_3, args=(obj,)).start()
# Then no notification is processed ...
self.assertEqual(self.notifications, [])

# Wait for a while to make sure the function has finished.
time.sleep(0.1)
# ... and an error was raised
self.assertEqual(self.exceptions, [(self.obj, "foo", 0, 3)])

self.flush_event_loop()
# ... but the attribute change was still applied.
self.assertEqual(self.obj.foo, 3)

notifications = self.notifications
self.assertEqual(len(notifications), 1)
async def test_notification_from_separate_thread(self):
# Given an asyncio ui handler
self.enterContext(use_asyncio_ui_handler(asyncio.get_event_loop()))

thread_id, event = notifications[0]
self.assertEqual(event, (obj, "foo", 0, 3))
# When we set obj.foo to 3 on a separate thread.
background_thread = threading.Thread(target=self.modify_obj)
background_thread.start()
self.addCleanup(background_thread.join)

ui_thread = trait_notifiers.ui_thread
self.assertEqual(thread_id, ui_thread)
# Then the notification will eventually be processed on the main
# thread.
await asyncio.wait_for(self.done.wait(), timeout=5.0)
self.assertEqual(
self.notifications,
[(threading.main_thread(), (self.obj, "foo", 0, 3))],
)
self.assertEqual(self.obj.foo, 3)


class TestMethodUINotifiers(BaseTestUINotifiers, unittest.TestCase):
""" Tests for dynamic notifiers with `dispatch='ui'` set by method call.
"""
class TestMethodUINotifiers(
BaseTestUINotifiers, unittest.IsolatedAsyncioTestCase

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, TIL about unittest.IsolatedAsyncioTestCase!

):
"""Tests for dynamic notifiers with `dispatch='ui'` set by method call."""

def obj_factory(self):
obj = CalledAsMethod()
obj.on_trait_change(self.on_foo_notifications, "foo", dispatch="ui")
return obj


class TestDecoratorUINotifiers(BaseTestUINotifiers, unittest.TestCase):
""" Tests for dynamic notifiers with `dispatch='ui'` set by decorator. """
class TestDecoratorUINotifiers(
BaseTestUINotifiers, unittest.IsolatedAsyncioTestCase
):
"""Tests for dynamic notifiers with `dispatch='ui'` set by decorator."""

def obj_factory(self):
return CalledAsDecorator(callback=self.on_foo_notifications)
37 changes: 18 additions & 19 deletions traits/trait_notifiers.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,27 +27,33 @@

# Global Data

# The thread ID for the user interface thread
ui_thread = -1
# The currently active handler for notifications that must be run on the UI
# thread, or None if no handler has been set.
_ui_handler = None

# The handler for notifications that must be run on the UI thread
ui_handler = None

def get_ui_handler():
"""
Return the current user interface thread handler.
"""
return _ui_handler


def set_ui_handler(handler):
""" Sets up the user interface thread handler.
"""
global ui_handler, ui_thread
global _ui_handler

ui_handler = handler
ui_thread = threading.current_thread().ident
_ui_handler = handler
mdickinson marked this conversation as resolved.
Show resolved Hide resolved


def ui_dispatch(handler, *args, **kw):
if threading.current_thread() == threading.main_thread():
handler(*args, **kw)
elif _ui_handler is None:
raise RuntimeError("No UI handler has been set.")
else:
ui_handler(handler, *args, **kw)
_ui_handler(handler, *args, **kw)


class NotificationExceptionHandlerState(object):
Expand Down Expand Up @@ -153,11 +159,7 @@ def _get_handlers(self):
thread.
"""
thread_local = self.thread_local
if isinstance(thread_local, dict):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive-by cleanup: this condition is never true in modern Python.

id = threading.current_thread().ident
handlers = thread_local.get(id)
else:
handlers = getattr(thread_local, "handlers", None)
handlers = getattr(thread_local, "handlers", None)

if handlers is None:
if self.main_thread is not None:
Expand All @@ -167,10 +169,7 @@ def _get_handlers(self):
self._log_exception, False, False
)
handlers = [handler]
if isinstance(thread_local, dict):
thread_local[id] = handlers
else:
thread_local.handlers = handlers
thread_local.handlers = handlers

return handlers

Expand Down Expand Up @@ -615,10 +614,10 @@ class FastUITraitChangeNotifyWrapper(TraitChangeNotifyWrapper):
"""

def dispatch(self, handler, *args):
if threading.current_thread().ident == ui_thread:
if threading.current_thread() == threading.main_thread():
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the fix that should have been part of #1740.

handler(*args)
else:
ui_handler(handler, *args)
_ui_handler(handler, *args)
Comment on lines 621 to +622

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also guard against _ui_handler being undefined here, as we do in ui_dispatch?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we definitely should! Now I have to go figure out why the tests aren't exercising this ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I think they are exercising this, but our tests for the exception don't distinguish between a RuntimeError with a nice message and a TypeError from trying to use None as a callable. I need to re-raise exceptions and catch them ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworked in cc8653e



class NewTraitChangeNotifyWrapper(TraitChangeNotifyWrapper):
Expand Down
8 changes: 8 additions & 0 deletions traits/traits_notifiers.pyi
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
from typing import Callable

_UI_Handler = Callable[..., None] | None


def get_ui_handler() -> _UI_Handler: ...

def set_ui_handler(handler: _UI_Handler) -> None: ...