Fix overlapped I/O defects in hwIo.base.IoBase - #20697
Draft
LeonarddeR wants to merge 1 commit into
Draft
Conversation
LeonarddeR
force-pushed
the
hwIoOverlappedIo
branch
from
August 18, 2026 20:18
477f9e3 to
dbc9a3a
Compare
Overlapped writes now wait for completion on a dedicated manual-reset event instead of the device handle. A read completing on the same handle no longer ends the wait while the write is still pending. IoBase.write raises OSError when a write fails instead of returning silently. close() cancels a pending write using the write OVERLAPPED, is safe to call repeatedly, and Bulk.close no longer double-closes its handles. A failure to start a background read is reported through onReadError instead of silently ending the read loop, and its stale completion routine registration is discarded. Part of nvaccess#20569. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
LeonarddeR
force-pushed
the
hwIoOverlappedIo
branch
from
August 18, 2026 20:28
dbc9a3a to
44c6517
Compare
Collaborator
Author
|
@seanbudd May I request a signed try build for this? I want to run this for a day or two with several displays, and unsigned try builds are not ideal for that. |
Member
|
I've pushed to |
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
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:
Part of #20569.
This addresses every
hwIodefect reported there.The Albatross driver finding stays tracked in the issue, because it cannot be validated without the hardware.
Summary of the issue:
NVDA talks to braille displays and other raw devices through one helper class:
hwIo.base.IoBase.The class always keeps a read pending in the background, so data from the device arrives as soon as the device sends it.
Writes go to the same device handle.
They are asynchronous: Windows takes over the write, and the code waits until Windows reports that the write has completed.
That wait targets the wrong object.
An asynchronous operation should carry its own event object, so a wait applies to exactly that operation.
IoBaseattaches no event.The wait then falls back to watching the device handle itself.
Windows signals the device handle when any operation on it completes, including the background read.
Microsoft documents this fallback as unsafe for precisely this situation, in the GetOverlappedResult remarks and in Synchronous and Overlapped Pipe I/O, which requires a manual-reset event object per operation.
The consequence: when the device sends data while a write is in progress, the completed read ends the write wait early.
Windows reports this through the wait function's return value ("the operation is incomplete"), but the code ignores that value.
So
write()returns while Windows still owns the write buffer and its bookkeeping structure.The next write reuses the structure while it is still busy.
Python may free the buffer while Windows is still reading from it.
A write that genuinely failed, for example because the device disconnected, looks like a success.
The audit in #20569 found three more problems in the same file:
The cancel call identifies the operation to cancel by its bookkeeping structure, and the code passes the structure of the read.
So the write is never cancelled.
Windows cancels only the operations issued with the exact structure passed to
CancelIoEx.Windows requires exactly one close per handle; once the handle value has been reissued, a second close silently invalidates an unrelated handle (CloseHandle).
The code does not check for this.
The read loop then stops silently: the device looks alive but never receives again.
Which devices this reaches:
HID braille displays are fully exposed; they use this write path and share one handle between reads and writes, so writing cells races incoming key presses.
Serial displays do not have the write race, because pyserial performs its own writes with its own event.
USB bulk devices write on a separate handle, but have the close problems.
Description of user facing changes:
None claimable directly.
Braille output to HID displays no longer races incoming input reports.
No concrete user-reported symptom has been traced to this, apart from some vague reports of braille display key input stalling intermittently, but I have never been able to reproduce this reliably.
Description of developer facing changes:
OSErrorfromIoBase.write, instead of returning silently.Driver code that unknowingly relied on failed writes passing unnoticed will now see the exception.
IoBase.closemore than once is now safe.onReadErrorcallback, the same way errors of a completed read already are.Description of development approach:
The event is created once, in
IoBase.__init__, as a manual-reset event.Manual-reset is required: a wait consumes an auto-reset event, after which the completion wait can block forever (OVERLAPPED members).
No reset per write is needed; Windows resets the event when a new operation starts.
The background read keeps no event, which is correct: its completion is delivered through a completion routine, and
ReadFileExignores the event field.Windows reports failure through a zero return value, with the error available via
GetLastError(GetOverlappedResult return value).On failure,
IoBase.writeraisesctypes.WinError(), the same way it already raises when the write cannot start at all.IoBase.closepasses the write's own structure toCancelIoEx, on the write handle.IoBase.closeacts only on the first call; later calls return immediately.Bulk.closemarks its two handles invalid after closing them, so its own close is repeat-safe too.IoBase._asyncRead.On failure, the error goes through the driver's
onReadErrorcallback, exactly as_ioDonehandles errors of a completed read.The completion routine registered for that read would never run, so its registration is removed.
These defects were found and this change was developed in a supervised Claude Fable session (see #20569 for provenance); the code has been human-reviewed and the testing below is real.
Testing strategy:
No unit tests are included.
The devices this code drives are hardware; a named-pipe test would exercise pipe semantics rather than those devices (see the checklist note).
The change is validated empirically:
Before this change,
write()returns the moment the read completes, with the write still pending and zero bytes reported.After it,
write()returns only when the write itself completes, with all bytes transferred.hwIo.baseover real named pipes, and covers the changed behavior directly: a write that pends while a read completes on the same handle does not return until the write itself completes; a write whose peer disappears mid-write raisesOSError; and repeatedclose()calls never hand the same handle value toCloseHandletwice.Known issues with pull request:
A driver that unknowingly relied on failed writes passing unnoticed now sees
OSErrorfromwrite().That is the defect being fixed, but it changes what such a driver observes on a flaky or disconnecting device.
Note that we don't have to treat this as API breaking. A failing write would most likely mean that the driver will crash later anyway.
Code Review Checklist: