Ensure the C++ UIA rate limited event handler flusher thread is definitely terminated when NVDA shutts down#16198
Conversation
…her thread function to make it clearer as to why the thread will wake for flushes or when it should stop. * Add the ability to terminate UIA rate limited event handler's flusher thread upon request via a new terminate API call. this allows NvDA to ensure that the flusher thread can be terminated, even if it may not release the rate limited event handler COM object reference for some reason. * UIAHandler.terminate: specifically terminate the rate limited event handler from NvDA's main thread, before trying to terminate the UIA MTA thread. This ensures that the c++ UIA rate limitied event handler flusher thread is definitely terminated, even if there may be further errors terminating the UIA MTA Python thread.
michaelDCurran
requested review from
seanbudd
and removed request for
a team
lukaszgo1
reviewed
Feb 20, 2024
Contributor
|
I assume it is not clear why the thread fails to end normally for the reporter of #16072 ? Would it be possible to debug this further, perhaps with additional logging? Obviously for 2024.1 it is important the thread gets terminated and does not block NVDA's main process, but perhaps for next release understanding the real cause may prove useful. |
Member
Author
|
I do not see any error in the provided logs so far unfortunately.
Of course if I do get more information which suggests further
investigation I shall do so.
Remember that no one is able to reproduce this except for the reporter
of #16072. So for now it is just trying to take precautions. Though we
can at least be sure that we never want a c++ thread to remain after the
main thread exits for anyone.
Message ID: ***@***.***>
|
Co-authored-by: Łukasz Golonka <lukasz.golonka@mailbox.org>
See test results for failed build of commit 189363a4d1 |
Adriani90
pushed a commit
to Adriani90/nvda
that referenced
this pull request
Mar 13, 2024
…itely terminated when NVDA shutts down (nvaccess#16198) Fixes nvaccess#16072 Summary of the issue: If any background threads remain after NVDA exits, the NVDA process will stay alive until those background threads complete. For Python threads, setting daemon to true on the thread causes Python to kill off the thread when the main thread exits. However, Python is unaware of C++ threads. One particular C++ thread which has proven to keep NvDA alive for at least one person (see nvaccess#16072) is the UIA rate limited event handler flusher thread. We should try much harder to ensure that this thread is terminated during NVDA exit so that it does not keep the NVDA process alive after the main thread exits. Description of user facing changes The NVDA process is less likely to stay around after NvDA has exited, which means that NVDA should no longer freeze on restart and NvDA updates should be less likely to fail due to NVDA still running. Description of development approach UIA rate limited event handler: improve logic and logging of the flusher thread function to make it clearer as to why the thread will wake for flushes or when it should stop. Add the ability to terminate UIA rate limited event handler's flusher thread upon request via a new terminate API call. This allows NvDA to ensure that the flusher thread can be terminated, even if it may not release the rate limited event handler COM object reference for some reason. UIAHandler.terminate: specifically terminate the rate limited event handler from NvDA's main thread, before trying to terminate the UIA MTA thread. This ensures that the c++ UIA rate limitied event handler flusher thread is definitely terminated, even if there may be further errors terminating the UIA MTA Python thread.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link to issue number:
Fixes #16072
Summary of the issue:
If any background threads remain after NVDA exits, the NVDA process will stay alive until those background threads complete. For Python threads, setting daemon to true on the thread causes Python to kill off the thread when the main thread exits. However, Python is unaware of C++ threads.
One particular C++ thread which has proven to keep NvDA alive for at least one person (see #16072) is the UIA rate limited event handler flusher thread.
We should try much harder to ensure that this thread is terminated during NVDA exit so that it does not keep the NVDA process alive after the main thread exits.
Description of user facing changes
The NVDA process is less likely to stay around after NvDA has exited, which means that NVDA should no longer freeze on restart and NvDA updates should be less likely to fail due to NVDA still running.
Description of development approach
Testing strategy:
Known issues with pull request:
None known.
Code Review Checklist: