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

source/braille.py

@@ -1983,7 +1983,6 @@ def __init__(self):
 
 		self.queuedWriteLock = threading.Lock()
 		self.ackTimerHandle = winKernel.createWaitableTimer()
-		self._ackTimeoutResetterApc = winKernel.PAPCFUNC(self._ackTimeoutResetter)
 
 		brailleViewer.postBrailleViewerToolToggledAction.register(self._onBrailleViewerChangedState)
 
@@ -2599,13 +2598,12 @@ def _bgThreadExecutor(self, param: int):
 			if self.display.receivesAckPackets:
 				self.display._awaitingAck = True
 				SECOND_TO_MS = 1000
-				winKernel.setWaitableTimer(
+				hwIo.bgThread.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),
-					0,
-					self._ackTimeoutResetterApc
+					self._ackTimeoutResetter
 				)
 
 	def _ackTimeoutResetter(self, param: int):

source/extensionPoints/util.py

@@ -7,6 +7,9 @@
 used, however for more advanced requirements these utilities can be used directly.
 """
 
+
+# "annotations" Needed to reference BoundMethodWeakref in one of the init params of itself.
+from __future__ import annotations
 import weakref
 import inspect
 from typing import (
@@ -41,11 +44,18 @@ class BoundMethodWeakref(Generic[HandlerT]):
 	"""
 	handlerKey: Tuple[int, int]
 
-	def __init__(self, target: HandlerT, onDelete):
-		def onRefDelete(weak):
-			"""Calls onDelete for our BoundMethodWeakref when one of the individual weakrefs (instance or function) dies.
-			"""
-			onDelete(self)
+	def __init__(
+			self,
+			target: HandlerT,
+			onDelete: Optional[Callable[[BoundMethodWeakref], None]] = None
+	):
+		if onDelete:
+			def onRefDelete(weak):
+				"""Calls onDelete for our BoundMethodWeakref when one of the individual weakrefs (instance or function) dies.
+				"""
+				onDelete(self)
+		else:
+			onRefDelete = None
 		inst = target.__self__
 		func = target.__func__
 		self.weakInst = weakref.ref(inst, onRefDelete)

source/hwIo/base.py

@@ -1,7 +1,7 @@
 # A part of NonVisual Desktop Access (NVDA)
 # This file is covered by the GNU General Public License.
 # See the file COPYING for more details.
-# Copyright (C) 2015-2018 NV Access Limited, Babbage B.V.
+# Copyright (C) 2015-2023 NV Access Limited, Babbage B.V., Leonard de Ruijter
 
 
 """Raw input/output for braille displays via serial and HID.
@@ -10,37 +10,55 @@
 See L{braille.BrailleDisplayDriver.isThreadSafe}.
 """
 
+# "annotations" Needed to provide the inner type for weakref.ReferenceType.
+from __future__ import annotations
 import sys
 import ctypes
 from ctypes import byref
 from ctypes.wintypes import DWORD
 from typing import Optional, Any, Union, Tuple, Callable
-
+import weakref
 import serial
 from serial.win32 import OVERLAPPED, FILE_FLAG_OVERLAPPED, INVALID_HANDLE_VALUE, ERROR_IO_PENDING, COMMTIMEOUTS, CreateFile, SetCommTimeouts
 import winKernel
 import braille
 from logHandler import log
 import config
 import time
+from .ioThread import IoThread, apcsWillBeStronglyReferenced
+import NVDAState
+
+
+def __getattr__(attrName: str) -> Any:
+	"""Module level `__getattr__` used to preserve backward compatibility."""
+	if attrName == "LPOVERLAPPED_COMPLETION_ROUTINE" and NVDAState._allowDeprecatedAPI():
+		log.warning(
+			"Importing LPOVERLAPPED_COMPLETION_ROUTINE from hwIo.base is deprecated. "
+			"Import LPOVERLAPPED_COMPLETION_ROUTINE from hwIo.ioThread instead."
+		)
+		from .ioThread import LPOVERLAPPED_COMPLETION_ROUTINE
+		return LPOVERLAPPED_COMPLETION_ROUTINE
+	raise AttributeError(f"module {repr(__name__)} has no attribute {repr(attrName)}")
 
-LPOVERLAPPED_COMPLETION_ROUTINE = ctypes.WINFUNCTYPE(None, DWORD, DWORD, serial.win32.LPOVERLAPPED)
 
 def _isDebug():
 	return config.conf["debugLog"]["hwIo"]
 
+
 class IoBase(object):
 	"""Base class for raw I/O.
 	This watches for data of a specified size and calls a callback when it is received.
 	"""
+	_ioThreadRef: weakref.ReferenceType[IoThread]
 
 	def __init__(
 			self,
 			fileHandle: Union[ctypes.wintypes.HANDLE],
 			onReceive: Callable[[bytes], None],
 			writeFileHandle: Optional[ctypes.wintypes.HANDLE] = None,
 			onReceiveSize: int = 1,
-			onReadError: Optional[Callable[[int], bool]] = None
+			onReadError: Optional[Callable[[int], bool]] = None,
+			ioThread: Optional[IoThread] = None,
 	):
 		"""Constructor.
 		@param fileHandle: A handle to an open I/O device opened for overlapped I/O.
@@ -50,8 +68,10 @@ def __init__(
 		@param writeFileHandle: A handle to an open output device opened for overlapped I/O.
 		@param onReceiveSize: The size (in bytes) of the data with which to call C{onReceive}.
 		@param onReadError: If provided, a callback that takes the error code for a failed read
-		  and returns True if the I/O loop should exit cleanly or False if an
-		  exception should be thrown
+			and returns True if the I/O loop should exit cleanly or False if an
+			exception should be thrown
+		@param ioThread: If provided, the I/O thread used for background reads.
+			if C{None}, defaults to L{hwIo.bgThread}
 		"""
 		self._file = fileHandle
 		self._onReceive = onReceive
@@ -61,18 +81,24 @@ def __init__(
 		self._readBuf = ctypes.create_string_buffer(onReceiveSize)
 		self._readOl = OVERLAPPED()
 		self._recvEvt = winKernel.createEvent()
-		self._ioDoneInst = LPOVERLAPPED_COMPLETION_ROUTINE(self._ioDone)
 		self._writeOl = OVERLAPPED()
+		if ioThread is None:
+			from . import bgThread as ioThread
+		self._ioThreadRef = weakref.ref(ioThread)
 		# Do the initial read.
 		self._initialRead()
 
 	def _initialRead(self):
-		"""Performs the initial background read by queuing it as an APC to the IO background thread.
-		This method is tied to the built-in i/o thread.
-		It can be overridden to do an initial read on a different thread.
+		"""Performs the initial background read by queuing it as an APC to the IO background thread
+		provided at initialization time.
 		"""
-		from . import bgThread
-		bgThread.queueAsApc(lambda param: self._asyncRead())
+		ioThread = self._ioThreadRef()
+		if not ioThread:
+			raise RuntimeError("I/O thread is no longer available")
+		if apcsWillBeStronglyReferenced:
+			ioThread.queueAsApc(self._asyncReadBackwardsCompat)
+		else:
+			ioThread.queueAsApc(self._asyncRead)
 
 	def waitForRead(self, timeout:Union[int, float]) -> bool:
 		"""Wait for a chunk of data to be received and processed.
@@ -137,11 +163,26 @@ def __del__(self):
 			if _isDebug():
 				log.debugWarning("Couldn't delete object gracefully", exc_info=True)
 
-	def _asyncRead(self):
+	def _asyncRead(self, param: Optional[int] = None):
+		ioThread = self._ioThreadRef()
+		if not ioThread:
+			raise RuntimeError("I/O thread is no longer available")
 		# Wait for _readSize bytes of data.
 		# _ioDone will call onReceive once it is received.
 		# onReceive can then optionally read additional bytes if it knows these are coming.
-		ctypes.windll.kernel32.ReadFileEx(self._file, self._readBuf, self._readSize, byref(self._readOl), self._ioDoneInst)
+		ctypes.windll.kernel32.ReadFileEx(
+			self._file,
+			self._readBuf,
+			self._readSize,
+			byref(self._readOl),
+			ioThread.getCompletionRoutine(self._ioDone)
+		)
+
+	if apcsWillBeStronglyReferenced:
+		def _asyncReadBackwardsCompat(self, param: Optional[int] = None):
+			"""Backwards compatible wrapper around L{_asyncRead} that calls it without param.
+			"""
+			self._asyncRead()
 
 	def _ioDone(self, error, numberOfBytes: int, overlapped):
 		if not self._onReceive:
@@ -173,21 +214,31 @@ def _notifyReceive(self, data: bytes):
 		except:
 			log.error("", exc_info=True)
 
+
 class Serial(IoBase):
 	"""Raw I/O for serial devices.
 	This extends pyserial to call a callback when data is received.
 	"""
 
 	def __init__(
-		self,
-		*args,
-		onReceive: Callable[[bytes], None],
-		**kwargs):
+			self,
+			*args,
+			onReceive: Callable[[bytes], None],
+			onReadError: Optional[Callable[[int], bool]] = None,
+			ioThread: Optional[IoThread] = None,
+			**kwargs
+	):
 		"""Constructor.
 		Pass the arguments you would normally pass to L{serial.Serial}.
-		There is also one additional required keyword argument.
+		There are also some additional keyword arguments (the first, onReceive, is required).
+
 		@param onReceive: A callable taking a byte of received data as its only argument.
 			This callable can then call C{read} to get additional data if desired.
+		@param onReadError: If provided, a callback that takes the error code for a failed read
+			and returns True if the I/O loop should exit cleanly or False if an
+			exception should be thrown
+		@param ioThread: If provided, the I/O thread used for background reads.
+			if C{None}, defaults to L{hwIo.bgThread}
 		"""
 		self._ser = None
 		self.port = args[0] if len(args) >= 1 else kwargs["port"]
@@ -202,7 +253,12 @@ def __init__(
 		self._origTimeout = self._ser.timeout
 		# We don't want a timeout while we're waiting for data.
 		self._setTimeout(None)
-		super(Serial, self).__init__(self._ser._port_handle, onReceive)
+		super().__init__(
+			self._ser._port_handle,
+			onReceive,
+			onReadError=onReadError,
+			ioThread=ioThread
+		)
 
 	def read(self, size=1) -> bytes:
 		data = self._ser.read(size)
@@ -257,16 +313,19 @@ def __init__(
 			self, path: str, epIn: int, epOut: int,
 			onReceive: Callable[[bytes], None],
 			onReceiveSize: int = 1,
-			onReadError: Optional[Callable[[int], bool]] = None
+			onReadError: Optional[Callable[[int], bool]] = None,
+			ioThread: Optional[IoThread] = None,
 	):
 		"""Constructor.
 		@param path: The device path.
 		@param epIn: The endpoint to read data from.
 		@param epOut: The endpoint to write data to.
 		@param onReceive: A callable taking a received input report as its only argument.
 		@param onReadError: An optional callable that handles read errors.
-		  It takes an error code and returns True if the error has been handled,
-		  allowing the read loop to exit cleanly, or False if an exception should be thrown.
+			It takes an error code and returns True if the error has been handled,
+			allowing the read loop to exit cleanly, or False if an exception should be thrown.
+		@param ioThread: If provided, the I/O thread used for background reads.
+			if C{None}, defaults to L{hwIo.bgThread}
 		"""
 		if _isDebug():
 			log.debug("Opening device %s" % path)
@@ -284,9 +343,14 @@ def __init__(
 			if _isDebug():
 				log.debug("Open write handle failed: %s" % ctypes.WinError())
 			raise ctypes.WinError()
-		super(Bulk, self).__init__(readHandle, onReceive,
-		                           writeFileHandle=writeHandle, onReceiveSize=onReceiveSize,
-		                           onReadError=onReadError)
+		super().__init__(
+			readHandle,
+			onReceive,
+			writeFileHandle=writeHandle,
+			onReceiveSize=onReceiveSize,
+			onReadError=onReadError,
+			ioThread=ioThread
+		)
 
 	def close(self):
 		super(Bulk, self).close()

source/hwIo/hid.py

@@ -12,7 +12,8 @@
 import ctypes
 from ctypes import byref
 from ctypes.wintypes import USHORT
-from typing import Tuple, Callable
+from typing import Tuple, Callable, Optional
+from .ioThread import IoThread
 
 from serial.win32 import FILE_FLAG_OVERLAPPED, INVALID_HANDLE_VALUE, CreateFile
 import winKernel
@@ -126,12 +127,24 @@ class Hid(IoBase):
 	"""
 	_featureSize: int
 
-	def __init__(self, path: str, onReceive: Callable[[bytes], None], exclusive: bool = True):
+	def __init__(
+			self,
+			path: str,
+			onReceive: Callable[[bytes], None],
+			exclusive: bool = True,
+			onReadError: Optional[Callable[[int], bool]] = None,
+			ioThread: Optional[IoThread] = None,
+	):
 		"""Constructor.
 		@param path: The device path.
 			This can be retrieved using L{hwPortUtils.listHidDevices}.
 		@param onReceive: A callable taking a received input report as its only argument.
 		@param exclusive: Whether to block other application's access to this device.
+		@param onReadError: An optional callable that handles read errors.
+			It takes an error code and returns True if the error has been handled,
+			allowing the read loop to exit cleanly, or False if an exception should be thrown.
+		@param ioThread: If provided, the I/O thread used for background reads.
+			if C{None}, defaults to L{hwIo.bgThread}
 		"""
 		if _isDebug():
 			log.debug("Opening device %s" % path)
@@ -168,7 +181,11 @@ def __init__(self, path: str, onReceive: Callable[[bytes], None], exclusive: boo
 		self._readSize = caps.InputReportByteLength
 		# Reading any less than caps.InputReportByteLength is an error.
 		super().__init__(
-			handle, onReceive, onReceiveSize=caps.InputReportByteLength
+			handle,
+			onReceive,
+			onReceiveSize=caps.InputReportByteLength,
+			onReadError=onReadError,
+			ioThread=ioThread
 		)
 
 	@property