Skip to content

Commit 0c0975d

Browse files
manuelblmanuelbl
committed
Robustness spec
1 parent 8bd4525 commit 0c0975d

1 file changed

Lines changed: 203 additions & 0 deletions

File tree

‎java-does-usb/robustness.md‎

Lines changed: 203 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,203 @@
1+
# Concurrency and Robustness Review — Windows Implementation
2+
3+
Review of the library's concurrency behavior, focused on the Windows implementation.
4+
Threads involved: user threads, the "USB async IO" completion thread (`WindowsAsyncTask`),
5+
and the "USB device monitor" thread (window message loop in `WindowsUsbDeviceRegistry`).
6+
7+
Findings are ordered by severity.
8+
9+
---
10+
11+
## 1. Potential permanent deadlock: completion handlers are invoked while holding the `WindowsAsyncTask` monitor — **FIXED (Windows & Linux)**
12+
13+
> **Status:** Fixed on 2026-07-03. `WindowsAsyncTask.completeTransfer` now removes the transfer
14+
> from the map, copies the results out of the OVERLAPPED, and recycles the OVERLAPPED under the
15+
> lock, then invokes the completion handler after releasing it, wrapped in try/catch so a throwing
16+
> handler cannot kill the async IO thread (partially addressing finding 3 as well). Verified with
17+
> the full hardware test suite (59 tests, all passing). macOS was already correct.
18+
>
19+
> The Linux counterpart is fixed the same way: `reapURBs` now reaps and unlinks transfers under the
20+
> task monitor, collecting them into a list, and invokes the handlers after releasing it (in a
21+
> `finally`, so already-reaped transfers still complete if reaping throws), each wrapped in
22+
> try/catch. `removeFromAsyncIOCompletion` is no longer method-synchronized: the epoll removal and
23+
> the stale-URB sweep each take the lock briefly, and the ENODEV completions are invoked outside
24+
> it. This is safe against a concurrent submit on the same device because `LinuxUsbDevice.close()`
25+
> and `LinuxUsbDevice.submitTransfer` serialize on the device monitor. Compile-verified only (see
26+
> finding 2's note on Linux verification).
27+
>
28+
> **Residual (pre-existing, Linux):** the ENODEV completions in `removeFromAsyncIOCompletion` run
29+
> on the closing thread, which holds the *device* monitor (`LinuxUsbDevice.close()` is
30+
> synchronized). A timed-out waiter holding a transfer monitor and calling `abortTransfers` (wants
31+
> the device monitor) while `close()` invokes that transfer's handler (wants the transfer monitor)
32+
> could still cycle. This inversion predates this fix and no longer involves the async IO thread,
33+
> so it can only stall the closing thread and the waiter, not the whole library. Windows doesn't
34+
> have it (close-triggered completions arrive via the IO thread, which holds no device monitor).
35+
36+
`WindowsAsyncTask.completeTransfer()` (`WindowsAsyncTask.java:157`) is `synchronized` and calls
37+
`transfer.completion().completed(transfer)` inside the lock. This creates a lock-order inversion
38+
with the user-thread paths:
39+
40+
- **User thread A** (sync transfer with timeout): holds the `transfer` monitor
41+
(`synchronized (transfer)` block in `WindowsUsbDevice.transferOut`, `WindowsUsbDevice.java:338`)
42+
and, when the timeout fires, calls `abortTransfers(...)` from inside `waitForTransfer`
43+
(`UsbDeviceImpl.java:385`) → **wants the device monitor** (`abortTransfers` is `synchronized`,
44+
`WindowsUsbDevice.java:482`).
45+
- **User thread B**: holds the device monitor (`submitTransferOut` is `synchronized`,
46+
`WindowsUsbDevice.java:413`) and calls `asyncTask.prepareForSubmission(...)` →
47+
**wants the asyncTask monitor**.
48+
- **Async IO thread**: holds the asyncTask monitor in `completeTransfer` and calls
49+
`onSyncTransferCompleted` → **wants A's transfer monitor** (`UsbDeviceImpl.java:477`).
50+
51+
If A's transfer completes right as its timeout expires (exactly the window in which timeouts race
52+
with completions), the cycle A→device→B, B→asyncTask→IO thread, IO thread→transfer→A closes and is
53+
permanent. Because `WindowsAsyncTask` is a process-wide singleton, a stuck IO thread freezes
54+
completions for **all** devices, and every no-timeout transfer then blocks forever in
55+
`waitNoTimeout`. The trigger is realistic: one thread doing timed transfers while another thread
56+
submits on the same or another device.
57+
58+
**Fix:** in `completeTransfer`, do the map removal / result copying / OVERLAPPED recycling under
59+
the lock, but invoke the completion handler *after* releasing it. (The macOS `MacosAsyncTask` has
60+
the same structural pattern and is worth the same check.)
61+
62+
## 2. Failed submission leaks the transfer registration — the Windows counterpart of the macOS fix is missing — **FIXED (Windows & Linux)**
63+
64+
> **Status:** Fixed on 2026-07-03. `WindowsAsyncTask.submissionFailed(transfer)` now undoes the
65+
> registration when a `WinUsb_*` call fails synchronously: it removes the map entry, returns the
66+
> OVERLAPPED to the pool, and clears the transfer's reference (safe because Win32 posts no
67+
> completion packet for a synchronous failure). Called from the error paths of all three submit
68+
> methods, mirroring the macOS fix. Verified with the full hardware test suite (59 tests, all
69+
> passing).
70+
>
71+
> The same leak was discovered and fixed in Linux: `LinuxAsyncTask.submitTransfer` called
72+
> `linkToUrb` (registers the URB→transfer mapping and takes a URB from the pool) and then threw on
73+
> `SUBMITURB` ioctl failure without cleanup. A private `submissionFailed(transfer)` (safe to call
74+
> under the already-held task monitor) now unlinks the transfer and recycles the URB — safe
75+
> because a URB rejected by the ioctl is never queued and will never be reaped. Compile-verified
76+
> only; the hardware test suite was run on Windows and does not exercise the Linux code path.
77+
78+
Commit `60a47af` ("Deregister transfer on exception (macOS)") added
79+
`asyncTask.submissionFailed(transfer)` when an async submission fails synchronously. Windows has
80+
the identical bug, unfixed: in `submitControlTransfer`, `submitTransferOut`, and `submitTransferIn`
81+
(`WindowsUsbDevice.java:404–408, 422–426, 440–444`), `asyncTask.prepareForSubmission(transfer)`
82+
registers the OVERLAPPED→transfer mapping and takes an OVERLAPPED from the pool; if `WinUsb_*` then
83+
fails with anything other than `ERROR_IO_PENDING`, the exception path leaves the entry in
84+
`requestsByOverlapped` forever and the OVERLAPPED never returns to `availableOverlappedStructs`.
85+
86+
Synchronous failures are the *normal* case right after an unplug, so an application that keeps
87+
retrying on a hot-unplugged device leaks an OVERLAPPED, a map entry, and the pinned transfer buffer
88+
per attempt.
89+
90+
**Fix:** add the same `submissionFailed()` cleanup to `WindowsAsyncTask` and call it from the three
91+
submit methods' error paths.
92+
93+
## 3. The singleton async IO thread can die, silently hanging the whole library — **FIXED (Windows & Linux)**
94+
95+
> **Status:** Fixed on 2026-07-03 (handler-exception protection was already added with finding 1;
96+
> macOS needed nothing — its upcall is fully wrapped and a run loop exit is logged).
97+
>
98+
> **Windows:** the completion loop body is wrapped in try/catch. Any exception — including a
99+
> `GetQueuedCompletionStatus` failure with a null OVERLAPPED, whose error text is also fixed —
100+
> now logs an ERROR, fails every transfer in `requestsByOverlapped` with
101+
> `ERROR_OPERATION_ABORTED` (handlers invoked outside the lock, individually guarded), marks the
102+
> task terminated, and exits. `prepareForSubmission` rejects new submissions with a clear
103+
> `UsbException` once terminated, so callers fail fast instead of hanging in `waitNoTimeout`.
104+
> The silent `return` on a successful dequeue with null OVERLAPPED ("registry closing?") was
105+
> speculative — nothing posts such packets — and now goes through the same fatal path.
106+
>
107+
> **Linux:** same structure — the epoll loop body is wrapped; a non-EINTR `epoll_wait` failure
108+
> (or an escaping exception) fails all pending transfers with `ECANCELED`, marks the task
109+
> terminated, and exits; `submitTransfer` then rejects new work. In addition, two per-device reap
110+
> failures no longer kill the thread: `EBADF` is treated as a benign race (the fd was closed
111+
> concurrently — reachable from an ordinary `close()` while the event thread already holds a
112+
> ready event) and any other unexpected reap errno degrades only that device (log, deregister
113+
> from epoll to avoid a hot loop, continue serving other devices).
114+
> `EPoll.removeFileDescriptor` now tolerates `EBADF` like `ENOENT`. `EBADF`/`ECANCELED` were
115+
> added to the committed jextract errno bindings by hand (asm-generic values, valid on
116+
> x64/ARM64) and to `gen_linux.sh` for the next regeneration on a Linux machine.
117+
>
118+
> Verified with the full hardware test suite on Windows (59 tests, all passing); the fatal paths
119+
> themselves and the Linux changes are verified by inspection/compilation only.
120+
121+
Two ways `asyncCompletionTask()` (`WindowsAsyncTask.java:68`) can terminate:
122+
123+
- `GetQueuedCompletionStatus` fails with a null OVERLAPPED → `throwLastError` (line 86) kills the
124+
thread.
125+
- Any `RuntimeException` escaping a completion handler — handlers run inline in this thread (see
126+
finding 1), so a bug or unexpected state in stream/user completion code is fatal to the loop.
127+
128+
There is no restart, and the failure mode is nasty: subsequent transfers submit fine but never
129+
complete, so callers block forever in `waitNoTimeout` (unbounded) and
130+
`flush()`/`waitForAvailableTransfer` (also unbounded by design).
131+
132+
**Fix:** wrap the loop body in a catch-log-continue (a failed handler must not stop dispatching
133+
other devices' completions) and reserve thread death for truly unrecoverable port errors — ideally
134+
then failing all pending transfers in `requestsByOverlapped` so waiters wake up.
135+
136+
Minor: the error text at line 86 says "SetupDiGetDeviceInterfaceDetailW" — a copy-paste from
137+
elsewhere.
138+
139+
## 4. `claimInterfaceSynchronized` leaves inconsistent state on error paths
140+
141+
In `WindowsUsbDevice.java:215–217`, `firstIntfHandle.deviceHandle` and `winusbHandle` are assigned
142+
**before** `asyncTask.addDevice(deviceHandle)`. If `addDevice` throws (CreateIoCompletionPort
143+
failure), the catch closes the device handle but leaves both fields set — a later claim sees
144+
`deviceHandle != null`, skips reopening, and submits I/O on a closed handle.
145+
146+
**Fix:** assign the fields only after `addDevice` succeeds (or null them in the catch, and
147+
`WinUsb_Free` the interface handle too).
148+
149+
Related: when claiming an associated interface (e.g. interface 1 of a function starting at 0) opens
150+
the device and then `WinUsb_GetAssociatedInterface` fails (line 228), the exception propagates with
151+
the device left open but `deviceOpenCount == 0` and no interface claimed. Since `close()` only
152+
releases *claimed* interfaces, that device handle (still registered with the completion port) leaks
153+
until process exit.
154+
155+
## 5. Visibility: shared flags are neither volatile nor consistently synchronized
156+
157+
- `showAsOpen`: written under the device monitor, but `isOpened()` (`WindowsUsbDevice.java:114`)
158+
reads it unlocked — `checkIsOpen`/`checkIsClosed` can act on stale state on a different thread.
159+
- `UsbDeviceImpl.connected`: written by the monitor thread in `disconnect()`, read unlocked via
160+
`isConnected()` and `checkIsClosed`.
161+
- `UsbDeviceRegistry.onDeviceConnectedHandler` / `onDeviceDisconnectedHandler`: set by the app
162+
thread, read by the monitor thread with no happens-before edge — a handler registered after
163+
`start()` may never be seen.
164+
165+
All are cheap to fix with `volatile`. Practical impact is low (stale reads, not corruption), but
166+
`isConnected()` returning `true` long after unplug is user-visible.
167+
168+
## 6. Minor observations
169+
170+
- **Data race on result fields in the timeout path**: `completeTransfer` writes
171+
`resultCode`/`resultSize` before taking the transfer monitor, while the timed-out waiter reads
172+
`transfer.resultCode()` holding it (`UsbDeviceImpl.java:384`). Consequences are benign (a
173+
redundant `WinUsb_AbortPipe`, or a timeout exception despite late data — inherent to the race
174+
anyway), but it's formally a data race.
175+
- **User callbacks run on the monitor thread inside the window procedure**
176+
(`WindowsUsbDeviceRegistry.java:326–347`). A slow `onDeviceConnected`/`onDeviceDisconnected`
177+
handler stalls all further device notifications (and the `disconnect()` cleanup of other
178+
devices). Exceptions are caught, which is good; the latency constraint is worth documenting if it
179+
isn't already.
180+
- `completeTransfer` dispatches all devices through one lock and one thread, so one slow handler
181+
delays every device's completions — currently fine since internal handlers are cheap queue
182+
operations, but it compounds finding 1's argument for invoking handlers outside the lock.
183+
184+
## What holds up well
185+
186+
The overall unplug/teardown story is sound: closing the WinUSB/file handles cancels pending I/O,
187+
cancelled I/O still posts completion packets to the port, so blocked waiters wake with an error
188+
result; the stream teardown paths (`close()`, `collectOutstandingTransfers`, abort-completion wait)
189+
are all deadline-bounded so a genuinely lost completion degrades to a logged warning rather than a
190+
hang — only the *non*-teardown unbounded waits remain exposed, and only via finding 3.
191+
192+
The `claimInterface` retry loop sleeps outside the device monitor, interrupts are deferred rather
193+
than swallowed, `transfer.wait()` correctly releases the transfer monitor while submissions on
194+
other threads proceed, and the copy-on-write device list with the case-insensitive Windows override
195+
applied to both add and remove paths is correct. The timed-out-transfer buffers using
196+
`Arena.ofAuto()` so an abandoned transfer can't have its buffer freed under a late completion is a
197+
well-handled detail.
198+
199+
## Recommended priority
200+
201+
Findings 1, 2 and 3 are fixed on all platforms (see above). Next up: finding 4
202+
(`claimInterfaceSynchronized` error-path state), then the `volatile` flags of finding 5.
203+
The Linux fixes for findings 1–3 still need a hardware test run on a Linux machine.

0 commit comments

Comments
 (0)