Don't block an app's main thread when sending typed characters to NVDA#11425
Merged
Conversation
…typed characters.
…n argument as it is never used.
See test results for failed build of commit 1ecbe0bb6b |
Collaborator
|
Just out of curiosity, have you ever considered moving to async RPC to avoid issues like this? |
LeonarddeR
approved these changes
Jul 27, 2020
Member
Author
|
I looked into async rpc, but it really is quite complex. On the client
side, for every call you need to initialize an async handle, and have
code that can handle the responce. On the server side, you need to call
a completion function at the end, and I think arguments are handled
differently. what we really need is simply just an rpc function that
returns straight away and doesn't care about handling a return value.
We currently use the ncalrpc protocol, which is certainly the fastest
protocol as a lot of it is at kernel level and it is written to be
purely local. The one draw back is that is not cancellable. If we used a
network protocol we could cancel, or we could use attributes in the idl
like message, or maybe, or idenpotent, but I think this type of protocl
may be slower. But most importantly: I'm not sure how easy it would be
to get the security right. We had to do a lot of complex work to support
Metro apps in windows 8.
Thus for now at least, I think queuing some of our rpc calls to a
non-main thread is the best way to go.
|
feerrenrut
suggested changes
Jul 27, 2020
feerrenrut
left a comment
Contributor
There was a problem hiding this comment.
Some small changes requested, but overall looks good!
feerrenrut
approved these changes
Jul 28, 2020
feerrenrut
left a comment
Contributor
There was a problem hiding this comment.
Thanks @michaelDCurran
20 tasks
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
5 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:
Re #11369 , #11398
Summary of the issue:
Currently, the main way that NVDA reports characters being typed is by detecting them in-process by watching for WM_CHAR window messages, and then sending the characters to NVDA via an RPC call.
However, the rpc call to NvDA is made directly from the app's main thread where the message was detected, and therefore blocks that thread until the rpc call is fully executed.
This can cause a deadlock between multiple threads in NVDA, bit more importantly a deadlock between NVDA and the app in question, if a cross-process COM object for the app happens to be released by Python's garbage collector within the incoming RPC call to NVDA, as this COM object's Release method will try and execute via the Windows message loop on the app's main thread, which is currently blocked by the rpc call.
Description of how this pull request fixes the issue:
Rather than making the RPC call directly, queue it to NVDA's in-process manager thread using APC (Asynchronous Procedure Calls) , and execute it there, no longer blocking the app's main thread.
Testing performed:
Known issues with pull request:
Not an issue, but just a note for future: In Windows 10 1607 and higher, we are able to detect typed characters within NVDA itself, rather than having to rely on in-process code at all. We should consider disabling the typed character support in-process completely on these more recent versions of Windows 10. However, right now it is more important to address the issue at its source, where it affects all Operating System versions.
Change log entry:
Bug fixes: