Decouple BgThread from braille module by LeonarddeR · Pull Request #14312 · nvaccess/nvda

LeonarddeR · 2022-10-29T13:34:22Z

Link to issue number:

Closes #14130
Slightly related to #14147

Summary of the issue:

In #14130, I raised the concern that the background thread for i/o is too tightly coupled too the braille module. This required the hwIo module to queue an APC to the braille background thread.

Description of user facing changes

None, apart from more safety checks that might impose stability improvements.

Description of development approach

Moved the background thread from the braille module. Instead make it a class on the hwIo module. It is also no longer a singleton and therefore add-ons can create their own i/o thread. I'm intending to use this in a new add-on.
Rather than all cases where APCs have to be queued to the background thread being responsible for wrapping themselves in a winKernel.PAPCFUNC, the queue function now takes a callable or lambda. The function is converted to an APC when queued. There is a cache on the thread class to keep references to wrapped APCs until they are finished. The wrapper also ensures that the APC doesn't execute when the thread is about to exit.
hwIo.base.IoBase now contains a new _initialRead method that initiates reading on the background thread. This method can be overridden on subclasses to outsource the reading to another thread.

Point 1 and 3 above together solve my concerns as noted in #14130

Testing strategy:

We need to thoroughly test braille display i/o for these changes.
I ran the pr build for several days without noticeable impact, in fact I was under the impression that braille initialization time decreased slightly with this approach.

Known issues with pull request:

This removes braille._BgThread and can be considered API breaking, however given the _BgThread class was underscore prefixed, it can be considered private code. This should be safe for 2023.1.

Change log entries:

For Developers

The singleton braille._BgThread class that provides background i/o for threadsafe braille display drivers has been moved to its own module in hwIo.ioThread. It is no longer a singleton. The single instance in NVDA core can be accessed using hwIo.bgThread, however add-on authors are encouraged to use their own instance when doing hardware i/o. (hwIo.base.IoBase relies on braille._BgThread #14130)

Code Review Checklist:

Pull Request description:
- description is up to date
- change log entries
Testing:
- Unit tests
- System (end to end) tests
- Manual testing
API is compatible with existing add-ons.
Documentation:
- User Documentation
- Developer / Technical Documentation
- Context sensitive help for GUI changes
UX of all users considered:
- Speech
- Braille
- Low Vision
- Different web browsers
- Localization in other languages / culture than English
Security precautions taken.

Co-authored-by: Sean Budd <seanbudd123@gmail.com>

…, the display might send an ack after closing the handle, resulting in an error

seanbudd · 2022-11-10T01:24:48Z

Converting to draft due to merge conflicts.

LeonarddeR · 2022-11-10T15:24:10Z

Solved merge conflicts and confirmed that stuff still works as expected.

feerrenrut

I'm going to need to go over this again. But there are a few things I picked up this time.

feerrenrut · 2022-11-16T09:29:06Z



-def _crashNVDA():
+def _crashNVDA(param: int = 0):


Can you add a docstring for this param? Or perhaps use a lambda or wrap the call to this to adapt it to the queueAsApc requirements.

feerrenrut · 2022-11-16T09:35:41Z

+		if self.thread:
+			return


It might be clearer if this class inherited from threading.Thread and the lifecycle of IoThread was managed by some owner.

Thanks, good idea. I applied that suggestion.

feerrenrut · 2022-11-16T09:40:06Z

-					_BgThread.ackTimerHandle,
-					int(handler.display.timeout*2000),
+					self.ackTimerHandle,
+					int(self.display.timeout * 2000),


Do you know about this value? Why is it multiplied by 2000?

It is converted to milli seconds and then multiplied by two. I added a descriptive comment.

This would be clearer if the two intentions were separated (IE, twice time out, convert to MS). Using named constants is a step better than a comment because the name is more likely to be updated to stay correct than a nearby comment.

SECOND_TO_MS = 1000 winKernel.setWaitableTimer( self.ackTimerHandle, # Wait twice the display driver timeout for acknowledgement packets # Note: timeout is in seconds whereas setWaitableTimer expects milliseconds int(self.display.timeout * 2 * SECOND_TO_MS ),

AppVeyorBot · 2022-11-16T18:35:59Z

PASS: Translation comments check.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/ftnnscyntx2m3u62/artifacts/output/nvda_snapshot_pr14312-27164,2fefdf05.exe
PASS: Lint check.
FAIL: System tests (tags: installer NVDA). See test results for more information.
PASS: Unit tests.
CI timing (mins):
INIT 0.0,
INSTALL_START 0.9,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 24.3,
TESTSETUP_START 0.0,
TESTSETUP_END 0.3,
TEST_START 0.0,
TEST_END 21.3,
FINISH_END 0.1

See test results for failed build of commit 2fefdf0582

feerrenrut · 2022-11-21T04:12:35Z

-					_BgThread.ackTimerHandle,
-					int(handler.display.timeout*2000),
+					self.ackTimerHandle,
+					int(self.display.timeout * 2000),


This would be clearer if the two intentions were separated (IE, twice time out, convert to MS). Using named constants is a step better than a comment because the name is more likely to be updated to stay correct than a nearby comment.

SECOND_TO_MS = 1000 winKernel.setWaitableTimer( self.ackTimerHandle, # Wait twice the display driver timeout for acknowledgement packets # Note: timeout is in seconds whereas setWaitableTimer expects milliseconds int(self.display.timeout * 2 * SECOND_TO_MS ),

feerrenrut · 2022-11-21T09:32:42Z

@LeonarddeR I've re-worded the developer change log entry, could you confirm the following is accurate:

- The singleton ``braille._BgThread`` class has been replaced with ``hwIo.ioThread.IoThread``. (#14130)
  - A single instance ``hwIo.bgThread`` (in NVDA core) of this class provides background i/o for thread safe braille display drivers.
  - This new class is not a singleton by design, add-on authors are encouraged to use their own instance when doing hardware i/o.
  -
-

LeonarddeR · 2022-11-21T10:19:40Z

Yes, this is fine!

…s are safe (#14627) Follow up of #14312 Summary of the issue: In #14312, we decoupled the background I/O thread from the braille module. However, the thread was still bound to IoBase in it's _initialRead method unless you'd override it. While I hoped the changes in #14312 would decrease crashing of braille, this was indeed the cause. However, crashes are still occurring here and there. I'm pretty sure I know the cause lies in the IO done completion routines that are still executed on the IO thread without ensuring that the instances of the routines still exist. I've seen this causing several access violation errors. Furthermore, #14312 saved bound methods in a dictionary on the iOThread. While this shouldn't be a big problem, this could cause potential issues with garbage collection (i.e. instances being kept alife forever because the APC of an instance was never called and it would be stuck in the cached apc dictionary). Description of user facing changes Hopefully, more stability. Description of development approach Added the ioThread as a constructor method to IoBase and derivatives as an optional argument. If not provided (default), the default IoThread is used. It will be added as a weak reference on the IoBase instance. Thereby it is no longer necessary to subclass IoBase or a derivative to override _initialRead to use another thread. Added IoThread.setWaitableTimer and IoThread.getCompletionRoutine, also ensuring that the function pointers are available when the background thread tries to call them. The function wrapper keeps a weak reference to the method it wraps. Thereby it ensures that a method of a died instance is no longer executed. IoThread.queueAsApc still stores a strong reference to the wrapped method on its APC, to keep backwards compatibility. This will be changed to weak references starting in NVDA 2024.1. API deprecation logic is in place. Removed the pre_IoThreadStop extension point. It was added in 2023.1 but never implemented and announced, so I think it is safe to do so.

@jcsteh

Related to #14899, #14312, #14627 Fixes #14895 Summary of the issue: Despite several attempts to fix this, NVDA's IoThread can crash without a clear cause. Description of user facing changes Less crashes, most likely, as tests indicate that this is the case. Description of development approach As proposed by @jcsteh , rather than creating a new function pointer for every APC or completion routine call, use a single internal APC and completion routine and use an internal cache to store the python functions, not the actual APC functions.

Decouple BgThread from braille module

99a95ce

LeonarddeR requested a review from a team as a code owner October 29, 2022 13:34

LeonarddeR requested a review from seanbudd October 29, 2022 13:34

This comment was marked as outdated.

Sign in to view

LeonarddeR added 2 commits October 29, 2022 17:08

Hopefully fix system test

fbfb775

Add test for apc queueing on hwIo thread

e87e269

This comment was marked as outdated.

Sign in to view

Rome slash from test name

1d5155e

seanbudd reviewed Nov 2, 2022

View reviewed changes

seanbudd requested a review from feerrenrut November 2, 2022 03:23

Apply suggestions from code review

725a54c

Co-authored-by: Sean Budd <seanbudd123@gmail.com>

This comment was marked as outdated.

Sign in to view

LeonarddeR and others added 3 commits November 2, 2022 10:45

Merge remote-tracking branch 'origin/master' into bgThreadRewrite

c70e3f1

Fix reference to typing.Optional

8f91ce3

Update source/braille.py

9a45ae5

Co-authored-by: Sean Budd <seanbudd123@gmail.com>

seanbudd approved these changes Nov 3, 2022

View reviewed changes

LeonarddeR added 2 commits November 3, 2022 14:39

Fix ack packet handling

67c6a29

Close the ack timer handle after terminating the display. If we don't…

793ff3a

…, the display might send an ack after closing the handle, resulting in an error

seanbudd marked this pull request as draft November 10, 2022 01:24

LeonarddeR added 2 commits November 10, 2022 07:49

Restore bdDetect

9a5faba

Merge remote-tracking branch 'origin/master' into bgThreadRewrite

1d4b6a1

LeonarddeR marked this pull request as ready for review November 10, 2022 15:24

feerrenrut suggested changes Nov 16, 2022

View reviewed changes

LeonarddeR added 2 commits November 16, 2022 17:58

Merge remote-tracking branch 'origin/master' into bgThreadRewrite

35a9b51

Review actions

22890e7

seanbudd added the merge-early Merge Early in a developer cycle label Nov 17, 2022

feerrenrut approved these changes Nov 21, 2022

View reviewed changes

LeonarddeR added 2 commits November 21, 2022 09:14

Move self.exit check out of try block

249a497

Add SECOND_TO_MS constant

6e33cdd

This comment was marked as outdated.

Sign in to view

feerrenrut approved these changes Nov 21, 2022

View reviewed changes

Update changes file for PR nvaccess#14312

76403a0

feerrenrut merged commit 9db8475 into nvaccess:master Nov 22, 2022

nvaccessAuto added this to the 2023.1 milestone Nov 22, 2022

burmancomp added a commit to burmancomp/nvda that referenced this pull request Nov 23, 2022

Modifications related to (PR nvaccess#14312)

e1ecd00

burmancomp mentioned this pull request Nov 28, 2022

Driver for Tivomatic Caiku Albatross 46/80 displays #13045

Merged

5 tasks

LeonarddeR mentioned this pull request Feb 11, 2023

Follow up of #14312: ensure IO completion routines and waitable timers are safe #14627

Merged

9 tasks

seanbudd mentioned this pull request May 9, 2023

Error in hardware support with some events in Thunderbird and other places #14895

Closed

LeonarddeR mentioned this pull request May 12, 2023

Use a single APC and completion routine #14924

Merged

6 tasks

LeonarddeR deleted the bgThreadRewrite branch August 23, 2025 06:27

Uh oh!

Conversation

LeonarddeR commented Oct 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of user facing changes

Description of development approach

Testing strategy:

Known issues with pull request:

Change log entries:

Code Review Checklist:

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

seanbudd commented Nov 10, 2022

Uh oh!

LeonarddeR commented Nov 10, 2022

Uh oh!

feerrenrut left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AppVeyorBot commented Nov 16, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

feerrenrut commented Nov 21, 2022

Uh oh!

LeonarddeR commented Nov 21, 2022 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

LeonarddeR commented Oct 29, 2022 •

edited

Loading