MAINT: add main thread check for ui dispatch, solve no ui failure #1740
Conversation
|
@homosapien-lcy Thank you for the PR. Please:
We should also check that the documentation makes the behaviour clear. |
traits/trait_notifiers.py
Outdated
| @@ -44,7 +44,7 @@ def set_ui_handler(handler): | |||
|
|
|||
|
|
|||
| def ui_dispatch(handler, *args, **kw): | |||
| if threading.current_thread().ident == ui_thread: | |||
| if threading.current_thread().ident == threading.main_thread().ident: | |||
There was a problem hiding this comment.
No need for the .ident here: you can just compare the thread objects directly.
| if threading.current_thread().ident == threading.main_thread().ident: | |
| if threading.current_thread() == threading.main_thread(): |
There was a problem hiding this comment.
Thanks for the suggestion. Are directly comparing threads vs comparing their ids completely equivalent?
There was a problem hiding this comment.
I think so, yes. The == check is essentially an is check (there's no __eq__ special method created), and there should be exactly one Thread object per OS thread.
So, for the test, I'm thinking of checking whether the handler triggered in the main thread is a "handler" and not in the main thread is a "ui_handler". Does that sound reasonable to you? And any suggested test that I can look at for reference? |
|
For the test, I suggest using something like Corran's example from the linked issue. The issue is about the behaviour of |
|
I've unchecked the "Update User manual" entry in the checklist: we should check whether the current documentation needs to be updated. In particular, the documentation should make it clear what the behaviour of ui dispatch is when there's no event loop running. |
|
@homosapien-lcy I'd suggest merging |
|
I found that the error raised by the ui_dispatch is happened not in the main thread, thus unable to be caught in the main thread, even if it says typeError, the unittest will still past...: So, what I also did is add a try except in ui_dispatch to throw a systemExit that will also notify the main thread. |
Both test and documentation are added |
|
Some tests failed (for instance, Tests/tests (ubuntu-latest, 3.8)) due to unable to download certain packages: |
|
@homosapien-lcy Thanks for the updates! For the test: this looks like a great start - the setup looks like exactly what we need. For the test itself, the behaviour we want to test is that the observer is executed when the trait value is changed. You should be able to change the observer to something that has an observable side-effect, and then test that side-effect to verify that the observer was called. (For example, set up an empty For the business logic: please could you explain the reason for the I haven't reviewed the documentation yet. |
This looks like an upstream problem that we've seen a few times before. I've restarted the test run. |
Looks like it was indeed an intermittent upstream problem - the re-run seems fine. |
Apologies - I see you already explained this in an earlier comment. Thank you. It's important to think through the consequences of any change to the business logic. A question for you: with this change, if I have a Traits-using application and some part of that application happens to cause an exception to be raised in a handler, what then happens to that application? What would have happened before this change? |
Thanks for the comments, I found a work around by adding an event list in the main thread and has the test handler add item to the event list, when the side thread failed, the list will be empty and cause an exception in the main thread. |
traits/tests/test_trait_notifiers.py
Outdated
| # Then | ||
| try: | ||
| t.test_param = 1 | ||
| except Exception: |
There was a problem hiding this comment.
Let's remove this try/except - it's just extra boilerplate and we don't really need it. If an exception is raised, the test will already fail, and there's no particular reason to expect an exception to be raised either before or after the change in this PR.
There was a problem hiding this comment.
I'd also move the t.test_param = 1 to the "When" block - it's the main part of the behaviour we're testing. The "Then" block is for testing the effects of that behaviour.
There was a problem hiding this comment.
It's often helpful to think about expanding out the "When/Then" to full sentence fragments: E.g., "When we change the value of a trait with a dispatch='ui' observer, then that observer is run as expected.
There was a problem hiding this comment.
Thanks! thats a great advice
traits/tests/test_trait_notifiers.py
Outdated
| self.fail("test_ui_dispatch raised an Exception unexpectedly!") | ||
|
|
||
| # also check the observer is called (test_handler function) | ||
| self.assertTrue(event_list) |
There was a problem hiding this comment.
I'd suggest a slightly stronger check, that the event list has length one. It would be even better to extract the event from the event list and check that the "new" has the expected value of 1.
There was a problem hiding this comment.
Stronger tests are added
There was a problem hiding this comment.
Thanks. Please could you check that the event list has length one, too? e.g.,, replace self.assertTrue(event_list) with self.assertEqual(len(event_list), 1)?
There was a problem hiding this comment.
Thanks for the suggestion, just updated
Just for clarity, there is no side thread here. Everything is happening in the main thread. |
Actually, this is a good point. It would be useful to have tests that check that we get the expected result when |
Thanks for the explanation, but I have a question then: if everything is happening in the main thread, why the "TypeError" here does not cause the test to fail? The original test looks like this: |
By default, the exception is caught and logged. What you're seeing here in the output is the logged message. See the code here: traits/traits/observation/_trait_event_notifier.py Lines 118 to 124 in dcbefb4 and here: traits/traits/observation/exception_handling.py Lines 37 to 49 in dcbefb4 In cases like these, a useful technique is to search for the error message in the codebase - a code search for "Exception occurred in traits notification handler" would have brought up the above location. You can easily verify that everything is happening in the main thread by adding a |
If I under stand you correctly: The exception already happened in the main thread but it is caught thus the test is not failed? And the suggestions in your comments are implemented |
Yes! But you don't need to take my word for it - you can verify this for yourself.
Please could you address this one? https://github.com/enthought/traits/pull/1740/files#r1173395668 |
|
@homosapien-lcy Please ignore my comments on the documentation. There are a number of changes needed there, but I think the easiest path is for me to make a commit to this branch with the necessary changes. |
|
I've added new documentation, and removed the documentation that was added to the migration section. |
There was a problem hiding this comment.
I think everything has now been addressed. I'll merge when the CI completes.
@homosapien-lcy Thank you for the fix.
Since #1788, we have only one test module that makes use of the Qt event loop. That test module contains tests for the behaviour of handlers that use `dispatch='ui'` mechanism to redispatch off-thread notifications to the ui thread. This PR reworks that test module, with some significant collateral damage along the way. In detail: - reworks that test module (`test_ui_notifiers`) to avoid the need for the Qt event loop; instead, it tests against a `ui_handler` based on asyncio, which redispatches to the running asyncio event loop - adds a `get_ui_handler` counterpart to `set_ui_handler`, and exposes both functions in `traits.api` - adds type hints for `get_ui_handler` and `set_ui_handler` - removes two public module globals from `trait_notifiers`: `ui_handler` has been made private, while `ui_thread` is removed altogether - fixes a bug where ui dispatch didn't do the right thing (PR #1740 was incomplete; this bug should have been caught at review time on that PR) - makes another couple of drive-by cleanups, removing a very old check for `threading.local()` being a dict (which it hasn't been in living memory), and tidying up some uses of thread identity.
Previously the ui_dispatch(handler, *args, **kw) function in traits/traits/trait_notifiers.py only checks current_thread().ident with ui_thread. But supposedly, this current thread id should be compared with the main thread (threading.main_thread()). This discrepancy caused a no ui failure in ui dispatch as mentioned in issue #1732. This PR attempts to resolve this failure. Closes #1732
Checklist
docs/source/traits_api_reference) (seems no need to change for this PR)docs/source/traits_user_manual) (seems no need to change for this PR)