Skip to content

Fix overlapped I/O defects in hwIo.base.IoBase - #20697

Draft
LeonarddeR wants to merge 1 commit into
nvaccess:masterfrom
LeonarddeR:hwIoOverlappedIo
Draft

Fix overlapped I/O defects in hwIo.base.IoBase#20697
LeonarddeR wants to merge 1 commit into
nvaccess:masterfrom
LeonarddeR:hwIoOverlappedIo

Conversation

@LeonarddeR

Copy link
Copy Markdown
Collaborator

Link to issue number:

Part of #20569.
This addresses every hwIo defect 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.
IoBase attaches 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:

  • Closing a device should cancel a write that is still pending.
    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.
  • Closing twice is not guarded, and the destructor closes again after an explicit close.
    Windows requires exactly one close per handle; once the handle value has been reissued, a second close silently invalidates an unrelated handle (CloseHandle).
  • Starting the background read can fail, for example when too many asynchronous operations are outstanding.
    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:

  • A failed write now raises OSError from IoBase.write, instead of returning silently.
    Driver code that unknowingly relied on failed writes passing unnoticed will now see the exception.
  • Calling IoBase.close more than once is now safe.
  • A background read that cannot start is now reported through the driver's onReadError callback, the same way errors of a completed read already are.

Description of development approach:

  • Every write now carries its own event object, so the completion wait applies to the write and nothing else.
    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 ReadFileEx ignores the event field.
  • The result of the completion wait is now checked.
    Windows reports failure through a zero return value, with the error available via GetLastError (GetOverlappedResult return value).
    On failure, IoBase.write raises ctypes.WinError(), the same way it already raises when the write cannot start at all.
  • Closing now cancels the pending write by naming the write operation: IoBase.close passes the write's own structure to CancelIoEx, on the write handle.
  • IoBase.close acts only on the first call; later calls return immediately.
    Bulk.close marks its two handles invalid after closing them, so its own close is repeat-safe too.
  • Starting the background read is now checked, in IoBase._asyncRead.
    On failure, the error goes through the driver's onReadError callback, exactly as _ioDone handles 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:

  • A standalone reproduction from hwIo: overlapped writes wait on the wrong object, and several I/O results go unchecked #20569 makes a write genuinely pend (a 256 KB write into a 1 KB pipe buffer) and completes a read while the write is outstanding.
    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.
  • The RDAccess add-on's test suite runs against this branch's real hwIo.base over 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 raises OSError; and repeated close() calls never hand the same handle value to CloseHandle twice.
  • Manual testing with a HID braille display is planned before marking this ready for review.

Known issues with pull request:

A driver that unknowingly relied on failed writes passing unnoticed now sees OSError from write().
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:

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

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

Copy link
Copy Markdown
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.

@seanbudd

Copy link
Copy Markdown
Member

I've pushed to try-hwIoOverlappedIo, builds will be push to https://download.nvaccess.org/snapshots/try/

@github-actions
github-actions Bot requested a deployment to snapshot August 20, 2026 06:43 Abandoned
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants