queueHandler: use SimpleQueue rather than Queue by michaelDCurran · Pull Request #11373 · nvaccess/nvda

michaelDCurran · 2020-07-14T04:43:51Z

Link to issue number:

Summary of the issue:

It is possible for both NVDA and an app (such as Microsoft Word) to both deadlock when typing characters.
The process is as follows:

The usertypes a character in the app.
NVDA's in-process code detects the typed character in the app's main thread and calls back into NVDA via rpc.
NVDA handles the incoming rpc call by queuing a typedCharacter event via eventHandler.queueEvent.
While NVDA is queuing the event function with Queue.put:
4.1. Python's garbage collector runs and or Python deletes objects that are no longer in scope.
4.2. One or more of these objects are comtypes COM objects marshalled from the app's main thread that sent the typed character notification.
4.3. Python calls Release on one of these COM objects which waits upon the app's main thread to release the object. Deadlock.

Even if this were addressed, a second deadlock is possible:

The usertypes a character in the app.
NVDA's in-process code detects the typed character in the app's main thread and calls back into NVDA via rpc.
NVDA handles the incoming rpc call by queuing a typedCharacter event via eventHandler.queueEvent.
queueEvent acquires a lock and increments counts for the event name etc so that eventHandler.isPendingEvents can tell there are events if it were to be called.
4.1. During the lock, Python's garbage collector runs and or Python deletes objects that are no longer in scope.
4.2. One or more of these objects are comtypes COM objects marshalled from the app's main thread that sent the typed character notification.
4.3. Python calls Release on one of these COM objects which waits upon the app's main thread to release the object. Deadlock.

Description of changes

queueHandler now uses a queue.SimpleQueue rather than a queue.Queue. SimpleQueue does not use locking, is unbounded and has no task management. We never needed any of that functionality anyways.
nvdaHelper.nvdaControlerInternal_typedCharacterNotify now manually queues a call to executeEvent rather than calling queueEvent.
Both of these changes ensure that no locks are entered during that particular incoming RPC call.

Testing performed

Tried to make MS Word / NVDA freeze by typing very fast into a document for at least 30 seconds. No freeze was experienced.

Known Issues

I don't believe there any drawbacks to these changes, though they may not address the issue as specifically as they could. The root of the problem still remains that

It seems to be impossible to cancel an IUnknown::release COM call.
The Python garbage collector and or out-of-scope deletion can occur at any time in any thread, and for comtypes, this may not be even the thread the COM object was marshalled to.

Changelog

Bug fixes:

NVDA should no longer sometimes freeze when rapidly typing in Microsoft Word documents.

…Queue is much more light-wait and reduces the chance of deadlocks.

…aracter notifications rather than using queueEvent as queueEvent may cause a deadlock with the app who sent the typed character notification if Python's garbage collector runs and releases a COM object from that process during queueEvent's lock.

LeonarddeR · 2020-07-14T11:03:06Z

 	focus=api.getFocusObject()
 	if focus.windowClassName!="ConsoleWindowClass":
-		eventHandler.queueEvent("typedCharacter",focus,ch=ch)
+		# Manually queue a call to executeEvent rather than using queueEvent,


Have you considered either one of the following:

Adding an additional kwarg to eventHandler.queueEvent that allows bypassing the pending events system. If that kwarg is semi private (e.g. by prefixing it with an underscore), it won't hurt anyone and it simplifies the API.

Tune the behavior of the garbage collector, i.e. by disabling the automatic collection and running the garbage collector manually at certain intervals, within the core pump, etc. Alternatively, we could temporarily disable collection in situations that could cause deadlocks.

https://docs.python.org/3/library/gc.html

bramd · 2020-07-14T12:06:47Z

@michaelDCurran First of all, glad to see something is done about this long standing issue. From the description I gather this must be a hell to debug/pinpoint the core issue.

Definitely out of scope for this PR, but I guess Python's garbage collector and COM can cause way more hard to catch issues. Ever considered moving COM-interaction out of the Python code base and into a low-level language like C/C++?

feerrenrut · 2020-07-14T13:32:14Z

More information about the introduction of SimpleQueue in Python 3.7: https://bugs.python.org/issue14976

feerrenrut · 2020-07-14T15:03:20Z

+		# As Currently queueEvent uses a lock and there is a small chance it could deadlock
+		# If garbage collection within the lock
+		# caused a COM object from the same thread as the typed character to be released.
+		queueHandler.queueFunction(


This bypasses the pendingEvents mechanism. Perhaps it is worth investigating disabling the garbage collection?

michaelDCurran · 2020-07-23T08:52:44Z

I think it is better to explicitly avoid queueEvent here, rather than changing queueEvent's functionality (even if based on a private argument) to bypass the pending events mechanism. Otherwise, queueEvent's functionality becomes harder to understand and predict.
Re garbage collection: I am currently working on another pr which greatly reduces the chance of reference cycles when interacting with Microsoft word, thus allowing Python to delete objects as soon as they go out of scope, rather than later on, possibly in the wrong thread. I agree that In the future, we should also consider managing calls to gc.collect and disabling automatic garbage collection.
I am going to merge this pr as is and deal with garbage collection separately.

feerrenrut · 2020-07-23T09:40:36Z

So this means that specifically for typed character events, the pending events mechanism isn't going to be used anymore? Why not do the same of all types of events?

LeonarddeR · 2020-07-23T11:18:14Z

Pending events is used for Focus, Caret, ValueChange and TextChange, I believe. Especially caret handling will probably break in a major way if we'd remove that mechanism.

feerrenrut · 2020-07-23T11:27:57Z

Prior to this PR it was also used for typed character events. I'd like to know why is this mechanism important for the events you listed, but not so for the typed character event. This now means that isPendingEvents will return False for "typedCharacter" events

michaelDCurran · 2020-07-23T21:46:45Z

I'm not too sure what more I can say other than that we have no code in NVDA that relies on asking if there are pending typed character events. Yet there is a great deal of code that relies on asking if there are pending gainFocus, caret and valueChange events. This change really is to avoid a very nasty freeze. I think that dropping the idea of tracking typedCharacter events while they are pending is worth it. If we can completely avoid the possibility of running garbage collection within the queueEvent lock in future, then I'm happy to re-instate this code.

…1373)" This reverts commit 45d7cc5.

…her than queueEvent (#11416) * Revert "reduce chance of freeze when typing characters in MS Word (#11373)" This reverts commit 45d7cc5. * Fix linting issues * Switch Queue to SimpleQueue * Add comment

michaelDCurran · 2020-07-23T23:23:31Z

I have reverted the typedCharacterNotify change in pr #11416, but kept the change from Queue to SimpleQueue. We can try and address the possible freeze in queueEvent when in typedCharacterNotify either with better garbage collection management, avoiding doing in-process typed character processing at all in Windows 10, or by somehow making the in-process part of typedCharacterNotify non-blocking.

codeofdusk · 2020-07-24T00:44:32Z

avoiding doing in-process typed character processing at all in Windows 10

This might be worth considering (we already do this for terminals). Can you think of any drawbacks?

michaelDCurran · 2020-07-26T22:30:36Z

We may be able to avoid in-process typedCharacter support entirely for newer versions of Windows 10, but previous to that we must still do in-process. However, we can change the in-process code to not block when doing this. I'm working on a pr for non-blocking in-process typedCharacter support currently.

codeofdusk · 2020-07-26T22:34:35Z

newer versions of Windows 10

ToUnicodeEx got the new flag on Windows 10 1607. So all Windows versions released in the past four years.

michaelDCurran · 2020-07-26T22:43:32Z

Correct. But not Windows 7, which we still have a lot of users for.

codeofdusk · 2020-07-26T22:50:00Z

Correct. But not Windows 7, which we still have a lot of users for.

Yes, but are users of an end of life OS going to update their screen reader? Do you have stats on this? (I imagine probably hard to get them as due to security issues lots of Win7 machines are run exclusively offline)

josephsl · 2020-07-26T23:13:34Z

Hi, surprisingly, yes, as evidenced by people using 2016 and 2017 releases of NVDA on older operating systems. I guess it won’t be until next year or 2022 when we need to think about Windows 7 support, but that won’t happen until a change in NVDA that will necessitate dropping support for anything under Windows 8.1. Thanks. From: Bill Dengler <notifications@github.com> Sent: Sunday, July 26, 2020 3:50 PM To: nvaccess/nvda <nvda@noreply.github.com> Cc: Subscribed <subscribed@noreply.github.com> Subject: Re: [nvaccess/nvda] queueHandler: use SimpleQueue rather than Queue (#11373) Correct. But not Windows 7, which we still have a lot of users for. Yes, but are users of an end of life OS <https://www.microsoft.com/en-us/microsoft-365/windows/end-of-windows-7-support> going to update their screen reader? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#11373 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB4AXEFI3WSBPSJERVNH4ETR5SXKPANCNFSM4OZEGFAA> .

michaelDCurran added 3 commits July 14, 2020 10:04

queuehandler: use queue.SimpleQueue rather than queue.Queue as Simple…

b6427e7

…Queue is much more light-wait and reduces the chance of deadlocks.

Fix linting issues

e1cb5d4

LeonarddeR approved these changes Jul 14, 2020

View reviewed changes

feerrenrut reviewed Jul 14, 2020

View reviewed changes

michaelDCurran merged commit 45d7cc5 into master Jul 23, 2020

michaelDCurran deleted the i11369 branch July 23, 2020 08:52

nvaccessAuto added this to the 2020.3 milestone Jul 23, 2020

michaelDCurran added a commit that referenced this pull request Jul 23, 2020

Revert "reduce chance of freeze when typing characters in MS Word (#1…

511fb9d

…1373)" This reverts commit 45d7cc5.

michaelDCurran mentioned this pull request Jul 23, 2020

Revert change to typedCharacterNotify using executeEvent directly rather than queueEvent #11416

Merged

michaelDCurran changed the title ~~reduce chance of freeze when typing characters in MS Word~~ queueHandler: use SimpleQueue rather than Queue Jul 23, 2020

LeonarddeR mentioned this pull request Jul 28, 2020

Fix qflushQueue trying to log an exception with queue name #11432

Merged

Uh oh!

Conversation

michaelDCurran commented Jul 14, 2020

Link to issue number:

Summary of the issue:

Description of changes

Testing performed

Known Issues

Changelog

Uh oh!

LeonarddeR Jul 14, 2020

Choose a reason for hiding this comment

Uh oh!

feerrenrut Jul 14, 2020

Choose a reason for hiding this comment

Uh oh!

bramd commented Jul 14, 2020

Uh oh!

feerrenrut commented Jul 14, 2020

Uh oh!

feerrenrut Jul 14, 2020

Choose a reason for hiding this comment

Uh oh!

michaelDCurran commented Jul 23, 2020

Uh oh!

feerrenrut commented Jul 23, 2020

Uh oh!

LeonarddeR commented Jul 23, 2020

Uh oh!

feerrenrut commented Jul 23, 2020

Uh oh!

michaelDCurran commented Jul 23, 2020 via email

Uh oh!

michaelDCurran commented Jul 23, 2020

Uh oh!

codeofdusk commented Jul 24, 2020

Uh oh!

michaelDCurran commented Jul 26, 2020 via email

Uh oh!

codeofdusk commented Jul 26, 2020

Uh oh!

michaelDCurran commented Jul 26, 2020 via email

Uh oh!

codeofdusk commented Jul 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josephsl commented Jul 26, 2020 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codeofdusk commented Jul 26, 2020 •

edited

Loading