fix(transport): the sync Connect() helpers no longer freeze a UI thread - #508
Conversation
…hread Daqifi.Core ships synchronous facades that block on their own async work (ConnectAsync().GetAwaiter().GetResult()). None of the awaits underneath them used ConfigureAwait(false), so a WPF/WinForms app calling DaqifiDeviceFactory.ConnectTcp(...) or device.Connect() on the UI thread froze permanently: the continuation was posted back to a context already blocked inside GetResult(), with no timeout and no exception. Every await in the library now uses ConfigureAwait(false), and CA2007 is enabled for src/Daqifi.Core so a naked await fails the build (warnings are already errors there). Test projects are exempt. closes #495 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/agentic_review |
PR Summary by QodoFix UI-thread deadlocks in sync connect helpers via ConfigureAwait(false) + CA2007
AI Description
Diagram
High-Level Assessment
Files changed (17)
|
Code Review by Qodo
1.
|
Qodo round 1: with the connect path resuming off the caller's context, a successful connect now reports status from a thread-pool thread rather than the UI thread. Drops and reconnects were already reported from background threads, so the contract was always "any thread" — it just was not written down. Says so on IStreamTransport, IUdpTransport and IDevice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 3dc1c18 |
…t give Qodo round 2: the remarks added in 3dc1c18 said a successful connect is reported from a thread-pool thread. ConfigureAwait(false) only declines the caller's context — an await on an already-completed task still resumes inline, so a dial that never suspends raises StatusChanged on the calling thread. The contract is "any thread"; the actionable rule is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit fce8613 |
|
Qodo-clean, CI green — ready for review. Round 3 on head Rounds 1 and 2 were both about the same thing and were both taken: the status events' threading contract. Round 1 pointed out it was undocumented; round 2 caught my own doc fix overclaiming that Full suite green on net9.0 (2928 Core + 43 Mcp) and net10.0 (2928), 0 failures, 0 warnings, on all three pushes. Not merging — this is for your review. |
The re-check is clean from my side: there are no active findings or unresolved review threads. Both earlier items are implemented: finding 1 corrected the inaccurate thread-pool guarantee, and finding 2 documented the arbitrary status-event thread affinity. The reported CI and freshness checks also support treating head |
What was wrong
A desktop app that called DAQiFi Core's synchronous connect helpers from its UI thread froze
solid.
DaqifiDeviceFactory.ConnectTcp(...),DaqifiDeviceFactory.ConnectSerial(...)anddevice.Connect()never returned, never timed out and never threw — the app was simply gone,with no error to report and nothing in a log to explain it. Serial did the same thing the
moment a retry engaged (a first
Open()that fails after a re-plug), which is exactly thesituation a user is in when they unplug and re-plug a device.
The cause is the classic sync-over-async deadlock. These helpers block the calling thread on
their own async work, and none of the awaits underneath them opted out of the caller's
SynchronizationContext. So the continuation was posted back to the UI thread, which wasalready blocked waiting for that very continuation. The WPF desktop migration is the first
consumer that would hit this.
How it was fixed
Every
awaitinDaqifi.Corenow usesConfigureAwait(false), so the library resumes on thethread pool rather than on the caller's context — and CA2007 is switched on for the library
project (
src/Daqifi.Core/.editorconfig), where warnings are already errors, so a nakedawaitcan no longer be reintroduced without failing the build. Test projects are exempt.Two things a reviewer may want to push back on:
reachable from a blocking facade" is a judgment call that goes stale on the next refactor,
and it is the wrong rule for a library anyway — even on a fully async path, resuming on the
UI context means the UI thread does the protobuf decoding and CSV export. Making the compiler
enforce the rule everywhere is what closes the second success criterion on bug(transport): sync Connect()/factory helpers deadlock under a SynchronizationContext — connect path lacks ConfigureAwait(false) #495.
await usingsites had to be restructured (SD-card parsers, firmware download,the SD download temp file).
await using var x = expr.ConfigureAwait(false)changesx'stype to
ConfiguredAsyncDisposable, so the variable is now declared first and the disposalscope wraps the body. Disposal order and the "dispose before the enclosing
catchruns"behaviour are unchanged; only the nesting moved.
Review also surfaced that the status events had no written threading contract — and that this
change is what makes it matter, since a successful connect used to land on the UI thread when
awaited from one.
IStreamTransport.StatusChanged,IUdpTransport.StatusChangedandIDevice.StatusChangednow say plainly that no particular thread is guaranteed and that a UIconsumer must marshal. Documentation only — marshalling notifications back to a captured
context is the exact coupling that caused this bug.
Not in scope: the wider
.editorconfigand style enforcement tracked by #484 — this adds onlythe one rule the bug needs, and #484 can extend the same file.
Verification
SynchronizationContextDeadlockTestsrun each blocking facade on a threadwith a UI-like single-threaded context installed that never pumps, and fail on a join timeout
instead of hanging the suite. Two of them are harness self-checks, including one that asserts
a naked
awaitdoes still deadlock — without it the rest could pass vacuously.src/Daqifi.Corereverted tomain,4 of the 5 real tests fail with the reported symptom (TCP connect/disconnect, serial
connect-with-retry, the retry executor's backoff delay, and
ConnectTcp). The UDP one passeseither way — its awaits complete synchronously — so it is a guard, not a regression catcher.
0 warnings.
/dev/cu.usbmodem1101: connect →--show-status→3 s @ 500 Hz → disconnect (1188 samples, this unit's known ~79% clock ratio),
--discover-serial(found Nq1,sn=9090539562006014104),--sd-list(45 files) and--sd-storage— the last two cover the SD text-exchange path this change touches. NoSD:GET, no delete/format, no reboot, no firmware, no LAN writes; serial only.closes #495
Not merging — opened for your review.