Skip to content

fix(transport): the sync Connect() helpers no longer freeze a UI thread - #508

Merged
tylerkron merged 3 commits into
mainfrom
fix/connect-path-configureawait-495
Aug 12, 2026
Merged

fix(transport): the sync Connect() helpers no longer freeze a UI thread#508
tylerkron merged 3 commits into
mainfrom
fix/connect-path-configureawait-495

Conversation

@tylerkron

@tylerkron tylerkron commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

What was wrong

A desktop app that called DAQiFi Core's synchronous connect helpers from its UI thread froze
solid. DaqifiDeviceFactory.ConnectTcp(...), DaqifiDeviceFactory.ConnectSerial(...) and
device.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 the
situation 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 was
already blocked waiting for that very continuation. The WPF desktop migration is the first
consumer that would hit this.

How it was fixed

Every await in Daqifi.Core now uses ConfigureAwait(false), so the library resumes on the
thread 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 naked
await can no longer be reintroduced without failing the build. Test projects are exempt.

Two things a reviewer may want to push back on:

  • The sweep is library-wide, not just the connect path. Enumerating "which awaits are
    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.
  • Eleven await using sites had to be restructured (SD-card parsers, firmware download,
    the SD download temp file). await using var x = expr.ConfigureAwait(false) changes x's
    type to ConfiguredAsyncDisposable, so the variable is now declared first and the disposal
    scope wraps the body. Disposal order and the "dispose before the enclosing catch runs"
    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.StatusChanged and
IDevice.StatusChanged now say plainly that no particular thread is guaranteed and that a UI
consumer must marshal. Documentation only — marshalling notifications back to a captured
context is the exact coupling that caused this bug.

Not in scope: the wider .editorconfig and style enforcement tracked by #484 — this adds only
the one rule the bug needs, and #484 can extend the same file.

Verification

  • 7 new tests in SynchronizationContextDeadlockTests run each blocking facade on a thread
    with 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 await does still deadlock — without it the rest could pass vacuously.
  • The tests were confirmed to catch the bug: with src/Daqifi.Core reverted to main,
    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 passes
    either way — its awaits complete synchronously — so it is a guard, not a regression catcher.
  • Full suite green on net9.0 (2928 Core + 43 Mcp) and net10.0 (2928), 0 failures,
    0 warnings.
  • Bench, non-destructive, fw 3.7.2 on /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. No
    SD:GET, no delete/format, no reboot, no firmware, no LAN writes; serial only.

closes #495

Not merging — opened for your review.

…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>
@tylerkron
tylerkron requested a review from a team as a code owner August 12, 2026 19:30
@tylerkron

Copy link
Copy Markdown
Contributor Author

/agentic_review

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Fix UI-thread deadlocks in sync connect helpers via ConfigureAwait(false) + CA2007

🐞 Bug fix 🧪 Tests ⚙️ Configuration changes 📝 Documentation 🕐 40+ Minutes

Grey Divider

AI Description

• Prevent sync-over-async deadlocks by using ConfigureAwait(false) across Daqifi.Core awaits.
• Enforce the rule with CA2007-as-error for the library project (tests exempt).
• Add deadlock-regression tests that simulate a never-pumped UI SynchronizationContext.
Diagram

graph TD
  UI(("UI thread")) --> Facades["Sync connect facades"] --> CoreAsync["Daqifi.Core async ops"] --> Pool(["ThreadPool continuations"])
  Analyzer[".editorconfig: CA2007"] -.-> CoreAsync
  Tests["Deadlock tests"] --> Facades
  Docs["CONTRIBUTING" ] --> Analyzer
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Deprecate/remove synchronous facades
  • ➕ Eliminates the primary sync-over-async deadlock vector entirely
  • ➕ Encourages correct async usage in UI applications
  • ➖ Breaking or behavior-changing for existing consumers
  • ➖ Doesn’t help callers that truly require synchronous APIs
2. Wrap sync facades with Task.Run to escape UI context
  • ➕ Localizes the fix to the synchronous entry points
  • ➕ Avoids sweeping changes across unrelated async code
  • ➖ Adds thread-pool scheduling overhead and can mask responsiveness issues
  • ➖ Still leaves library awaits potentially resuming on caller context for async callers
  • ➖ Harder to reason about cancellation/timeouts and can introduce starvation under load
3. Use a dedicated async context/dispatcher helper (e.g., AsyncEx) inside sync facades
  • ➕ Robust pattern for safely blocking on async without deadlock
  • ➕ Keeps caller-context concerns away from the rest of the library
  • ➖ Introduces a new dependency and conceptual complexity
  • ➖ Still doesn’t address unnecessary UI-context resumption for fully-async call paths

Recommendation: The PR’s approach (ConfigureAwait(false) library-wide + CA2007 enforcement) is the most robust for a reusable library: it fixes the immediate deadlock in the sync facades and prevents regressions, while also avoiding accidental continuation on a UI context for purely-async consumers. Consider deprecating sync facades separately if the API surface can move toward fully-async, but the current change is the correct, low-surprise fix for #495.

Files changed (17) +452 / -104

Bug fix (14) +128 / -104
ConnectRetryExecutor.csAvoid context capture in retry backoff and connect attempt awaits +2/-2

Avoid context capture in retry backoff and connect attempt awaits

• Applies ConfigureAwait(false) to the backoff delay and connectAttempt awaits so retry logic cannot deadlock a blocked SynchronizationContext. This is a key shared path across transports.

src/Daqifi.Core/Communication/Transport/ConnectRetryExecutor.cs

SerialStreamTransport.csUse ConfigureAwait(false) throughout serial connect/disconnect paths +5/-5

Use ConfigureAwait(false) throughout serial connect/disconnect paths

• Ensures serial connect overloads and retry execution do not capture the caller context, including retry completion and no-op async returns. Prevents deadlocks especially when retry engages after a failed Open() scenario.

src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs

TcpStreamTransport.csUse ConfigureAwait(false) throughout TCP connect/disconnect paths +6/-6

Use ConfigureAwait(false) throughout TCP connect/disconnect paths

• Applies ConfigureAwait(false) to connect overload chaining, timeout waits, and retry execution. Ensures synchronous Connect/Disconnect wrappers cannot deadlock a UI thread waiting on async work.

src/Daqifi.Core/Communication/Transport/TcpStreamTransport.cs

UdpTransport.csUse ConfigureAwait(false) for UDP send/receive and open/close async calls +7/-7

Use ConfigureAwait(false) for UDP send/receive and open/close async calls

• Applies ConfigureAwait(false) to SendAsync/ReceiveAsync and other awaited operations to avoid SynchronizationContext capture during UDP IO. Keeps sync Open/Close behavior safe under UI contexts.

src/Daqifi.Core/Communication/Transport/UdpTransport.cs

SerialDeviceFinder.csAvoid context capture during serial discovery waits and timeouts +4/-4

Avoid context capture during serial discovery waits and timeouts

• Adds ConfigureAwait(false) to semaphore acquisition, wake-up delays, timeout Task.WhenAny, and DTR settle delay. Prevents device discovery from resuming on a blocked caller context.

src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs

WiFiDeviceFinder.csAvoid context capture in WiFi discovery orchestration and receive loops +5/-5

Avoid context capture in WiFi discovery orchestration and receive loops

• Adds ConfigureAwait(false) across discovery overloads, semaphore waits, UDP send, Task.WhenAll, and receive awaits. Ensures discovery doesn’t marshal continuations back to UI threads.

src/Daqifi.Core/Device/Discovery/WiFiDeviceFinder.cs

NetworkConfigurationOperations.csAvoid context capture during WiFi module restart delay +1/-1

Avoid context capture during WiFi module restart delay

• Applies ConfigureAwait(false) to the post-config restart delay so network configuration updates cannot deadlock a blocked caller context.

src/Daqifi.Core/Device/Network/NetworkConfigurationOperations.cs

SdCardCsvFileParser.csRestructure await using and apply ConfigureAwait(false) in CSV SD parsing +5/-2

Restructure await using and apply ConfigureAwait(false) in CSV SD parsing

• Reworks await using to allow ConfigureAwait(false) on async disposal while keeping disposal scope equivalent. Ensures ParseAsync continuation and disposal don’t capture the caller SynchronizationContext.

src/Daqifi.Core/Device/SdCard/SdCardCsvFileParser.cs

SdCardFileParser.csRestructure await using and apply ConfigureAwait(false) in SD parsing +5/-2

Restructure await using and apply ConfigureAwait(false) in SD parsing

• Reworks async file stream disposal to use ConfigureAwait(false) and applies it to parsing awaits. Keeps the same disposal semantics while preventing UI-context capture.

src/Daqifi.Core/Device/SdCard/SdCardFileParser.cs

SdCardFileParserFactory.csApply ConfigureAwait(false) to SD parser factory async entry points +6/-3

Apply ConfigureAwait(false) to SD parser factory async entry points

• Restructures await using for the factory-created FileStream and applies ConfigureAwait(false) to parsing calls. Ensures the factory helper is safe under sync-over-async callers.

src/Daqifi.Core/Device/SdCard/SdCardFileParserFactory.cs

SdCardJsonFileParser.csRestructure await using and apply ConfigureAwait(false) in JSON SD parsing +5/-2

Restructure await using and apply ConfigureAwait(false) in JSON SD parsing

• Reworks await using to keep async disposal while applying ConfigureAwait(false). Applies ConfigureAwait(false) to parsing await so UI contexts are not captured.

src/Daqifi.Core/Device/SdCard/SdCardJsonFileParser.cs

SdCardOperations.csApply ConfigureAwait(false) across SD-card SCPI operations and downloads +18/-15

Apply ConfigureAwait(false) across SD-card SCPI operations and downloads

• Adds ConfigureAwait(false) to command execution helpers and several device settle delays. Restructures temp-file download await using to support ConfigureAwait(false) on async disposal without changing disposal order.

src/Daqifi.Core/Device/SdCard/SdCardOperations.cs

GitHubFirmwareDownloadService.csApply ConfigureAwait(false) and restructure async disposals in firmware downloads +43/-34

Apply ConfigureAwait(false) and restructure async disposals in firmware downloads

• Adds ConfigureAwait(false) across GitHub API calls, stream reads/writes, and download helpers. Restructures await using blocks for content/file streams to keep async disposal compatible with ConfigureAwait(false).

src/Daqifi.Core/Firmware/GitHubFirmwareDownloadService.cs

CsvExporter.csAvoid context capture during CSV export streaming and writing +16/-16

Avoid context capture during CSV export streaming and writing

• Applies ConfigureAwait(false) to header writes, sample streaming (await foreach), and row writes. Prevents CPU/IO-heavy export work from resuming on a UI thread context.

src/Daqifi.Core/Logging/Export/CsvExporter.cs

Tests (1) +300 / -0
SynchronizationContextDeadlockTests.csAdd regression tests for UI-thread deadlock scenario (#495) +300/-0

Add regression tests for UI-thread deadlock scenario (#495)

• Introduces a custom blocked SynchronizationContext harness to reproduce sync-over-async deadlocks deterministically. Adds transport, retry, and factory-level tests to ensure blocking facades complete (or throw) under a never-pumped UI-like context.

src/Daqifi.Core.Tests/Communication/Transport/SynchronizationContextDeadlockTests.cs

Documentation (1) +9 / -0
CONTRIBUTING.mdDocument ConfigureAwait(false) requirement for Daqifi.Core +9/-0

Document ConfigureAwait(false) requirement for Daqifi.Core

• Adds contributor guidance explaining why Daqifi.Core awaits must use ConfigureAwait(false) due to synchronous facades that can deadlock UI threads. Points to the library-only CA2007 enforcement and notes tests are exempt.

CONTRIBUTING.md

Other (1) +15 / -0
.editorconfigEnable CA2007 for Daqifi.Core to prevent naked awaits +15/-0

Enable CA2007 for Daqifi.Core to prevent naked awaits

• Adds a library-scoped .editorconfig enabling CA2007 so missing ConfigureAwait(false) is caught during build. Includes rationale focused on synchronous facades and UI SynchronizationContext deadlocks.

src/Daqifi.Core/.editorconfig

@qodo-code-review

qodo-code-review Bot commented Aug 12, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Threadpool claim inaccurate ✓ Resolved 🐞 Bug ⚙ Maintainability ⭐ New
Description
The XML remarks for IStreamTransport.StatusChanged/IDevice.StatusChanged claim that a successful
connect is reported from a thread-pool thread, but the connect path can complete synchronously and
raise StatusChanged inline on the calling thread. This makes the public contract misleading for
consumers that may rely on a thread-pool guarantee (e.g., to avoid doing work on a UI thread).
Code

src/Daqifi.Core/Communication/Transport/IStreamTransport.cs[R35-39]

+    /// watchdog or reader thread that detected it, and the connect path deliberately resumes off
+    /// the caller's <see cref="SynchronizationContext"/> so the synchronous
+    /// <see cref="Connect"/>/<see cref="Disconnect"/> facades cannot deadlock a UI thread
+    /// (issue #495) — so a successful connect is reported from a thread-pool thread too. A UI
+    /// consumer must marshal to its own dispatcher before touching controls.
Relevance

●●● Strong

Team often accepts correcting misleading XML contracts; thread/behavior guarantees should match
actual execution semantics.

PR-#435
PR-#357
PR-#98

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The documentation promises thread-pool delivery on successful connect, yet the implementation raises
the status change immediately after awaiting the connect attempt/transport connect task; if that
awaited task is already completed (common on first-attempt success, e.g., via Task.CompletedTask
in some transport paths such as SerialStreamTransport), the await does not suspend and the
continuation runs synchronously on the current (caller) thread. Since DaqifiDevice raises
StatusChanged when Status is updated and it transitions to Connected directly after the await,
the event can be raised on the calling thread, contradicting the remarks’ thread-pool wording.

src/Daqifi.Core/Communication/Transport/IStreamTransport.cs[32-40]
src/Daqifi.Core/Communication/Transport/ConnectRetryExecutor.cs[63-75]
src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[303-342]
src/Daqifi.Core/Device/IDevice.cs[45-53]
src/Daqifi.Core/Device/DaqifiDevice.cs[1459-1471]
src/Daqifi.Core/Device/DaqifiDevice.cs[1550-1554]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The XML documentation for `IStreamTransport.StatusChanged` and `IDevice.StatusChanged` currently states (or strongly implies) that a successful connect will be reported from a thread-pool thread. This is not guaranteed because awaiting a connect task (even with `ConfigureAwait(false)`) does not force an asynchronous hop; when the awaited task completes synchronously, the continuation (and thus the `StatusChanged` notification) runs inline on the calling thread.

## Issue Context
Connect flows using `ConnectRetryExecutor` can complete synchronously (for example, a first-attempt success where the connectAttempt returns a completed task), causing `onStatusChanged(true, null)` to execute inline. Similarly, `DaqifiDevice` sets `Status = Connected` (which raises `StatusChanged`) immediately after awaiting `_transport.ConnectAsync(...).ConfigureAwait(false)`; if that connect task completes without suspending (e.g., returns `Task.CompletedTask` after synchronous work), the status update and event run on the caller thread, contradicting any “thread-pool thread” guarantee.

## Fix Focus Areas
- src/Daqifi.Core/Communication/Transport/IStreamTransport.cs[32-40]
- src/Daqifi.Core/Device/IDevice.cs[45-53]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Undocumented event thread-affinity ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
Because ConnectRetryExecutor now uses ConfigureAwait(false), transport status notifications may
execute off the caller’s SynchronizationContext (often on a thread-pool thread) whenever the connect
path actually awaits asynchronously. IStreamTransport.StatusChanged/transport OnStatusChanged do not
document or enforce any thread-affinity, so consumers that previously relied on UI-thread delivery
can break without explicit guidance.
Code

src/Daqifi.Core/Communication/Transport/ConnectRetryExecutor.cs[R73-75]

+                await connectAttempt(options, cancellationToken).ConfigureAwait(false);
                onStatusChanged(true, null);
                return; // Success!
Relevance

●●● Strong

Team has accepted threading-contract documentation clarifications; ConfigureAwait(false) changes
callback context so documenting affinity is expected.

PR-#435

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The executor’s post-await callback (onStatusChanged) is now reached via a ConfigureAwait(false)
continuation, and the transports propagate that callback to StatusChanged via a direct Invoke
with no dispatching/marshaling; meanwhile the interface event docs don’t describe any threading
behavior.

src/Daqifi.Core/Communication/Transport/ConnectRetryExecutor.cs[69-75]
src/Daqifi.Core/Communication/Transport/TcpStreamTransport.cs[456-464]
src/Daqifi.Core/Communication/Transport/IStreamTransport.cs[24-33]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`ConnectRetryExecutor` now awaits with `ConfigureAwait(false)`, so code after the await (including `onStatusChanged(...)`) may run without the caller’s `SynchronizationContext`. The transports raise `StatusChanged` directly with no marshaling, but the public API docs don’t state that handlers may be invoked on arbitrary threads.

### Issue Context
This is likely an intentional change to prevent sync-over-async deadlocks, but it is also a compatibility change for UI apps that subscribed and updated UI controls directly. The best remediation is to explicitly document the threading contract (and optionally mention it in release notes) so consumers know to dispatch.

### Fix Focus Areas
- src/Daqifi.Core/Communication/Transport/IStreamTransport.cs[24-33]
- src/Daqifi.Core/Communication/Transport/TcpStreamTransport.cs[456-464]
- src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[684-692]
- src/Daqifi.Core/Communication/Transport/UdpTransport.cs[252-260]
- src/Daqifi.Core/Communication/Transport/ConnectRetryExecutor.cs[69-75]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Tip of the day
💡 Did you know, you can enable the Remediation agent and Qodo fixes findings in a dedicated fix PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Previous review results

Review updated until commit fce8613

Results up to commit 0a9600f ⚖️ Balanced


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Remediation recommended
1. Undocumented event thread-affinity ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
Because ConnectRetryExecutor now uses ConfigureAwait(false), transport status notifications may
execute off the caller’s SynchronizationContext (often on a thread-pool thread) whenever the connect
path actually awaits asynchronously. IStreamTransport.StatusChanged/transport OnStatusChanged do not
document or enforce any thread-affinity, so consumers that previously relied on UI-thread delivery
can break without explicit guidance.
Code

src/Daqifi.Core/Communication/Transport/ConnectRetryExecutor.cs[R73-75]

+                await connectAttempt(options, cancellationToken).ConfigureAwait(false);
                onStatusChanged(true, null);
                return; // Success!
Relevance

●●● Strong

Team has accepted threading-contract documentation clarifications; ConfigureAwait(false) changes
callback context so documenting affinity is expected.

PR-#435

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The executor’s post-await callback (onStatusChanged) is now reached via a ConfigureAwait(false)
continuation, and the transports propagate that callback to StatusChanged via a direct Invoke
with no dispatching/marshaling; meanwhile the interface event docs don’t describe any threading
behavior.

src/Daqifi.Core/Communication/Transport/ConnectRetryExecutor.cs[69-75]
src/Daqifi.Core/Communication/Transport/TcpStreamTransport.cs[456-464]
src/Daqifi.Core/Communication/Transport/IStreamTransport.cs[24-33]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`ConnectRetryExecutor` now awaits with `ConfigureAwait(false)`, so code after the await (including `onStatusChanged(...)`) may run without the caller’s `SynchronizationContext`. The transports raise `StatusChanged` directly with no marshaling, but the public API docs don’t state that handlers may be invoked on arbitrary threads.

### Issue Context
This is likely an intentional change to prevent sync-over-async deadlocks, but it is also a compatibility change for UI apps that subscribed and updated UI controls directly. The best remediation is to explicitly document the threading contract (and optionally mention it in release notes) so consumers know to dispatch.

### Fix Focus Areas
- src/Daqifi.Core/Communication/Transport/IStreamTransport.cs[24-33]
- src/Daqifi.Core/Communication/Transport/TcpStreamTransport.cs[456-464]
- src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[684-692]
- src/Daqifi.Core/Communication/Transport/UdpTransport.cs[252-260]
- src/Daqifi.Core/Communication/Transport/ConnectRetryExecutor.cs[69-75]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Qodo Logo

Comment thread src/Daqifi.Core/Communication/Transport/ConnectRetryExecutor.cs
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>
@tylerkron

Copy link
Copy Markdown
Contributor Author

/agentic_review

Comment thread src/Daqifi.Core/Communication/Transport/IStreamTransport.cs Outdated
@qodo-code-review

Copy link
Copy Markdown

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>
@tylerkron

Copy link
Copy Markdown
Contributor Author

/agentic_review

@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit fce8613

@tylerkron

Copy link
Copy Markdown
Contributor Author

Qodo-clean, CI green — ready for review.

Round 3 on head fce8613: no new findings (Bugs 0 / Rule violations 0 / Skill insights 0), 0 unresolved review threads, both earlier findings struck ✓ Resolved. Freshness confirmed two ways — the bot's "updated up to the latest commit fce8613" note, and 20 SHA references to fce8613 against 1 each to the two older commits. Settle re-check at +5 min: the summary comment was byte-identical (diff -q) and threads still 0. CI build SUCCESS on fce8613; MERGEABLE / CLEAN.

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 ConfigureAwait(false) guarantees a thread-pool hop, which it does not. Both fixes are XML doc comments only — git diff 0a9600f..fce8613 -- src/Daqifi.Core/ contains no non-comment line — so the bench result from the production commit still stands and was not re-run.

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.

@qodo-code-review

Copy link
Copy Markdown

Qodo-clean, CI green — ready for review.

Round 3 on head fce8613: no new findings (Bugs 0 / Rule violations 0 / Skill insights 0), 0 unresolved review threads, both earlier findings struck ✓ Resolved. Freshness confirmed two ways — the bot's "updated up to the latest commit fce8613" note, and 20 SHA references to fce8613 against 1 each to the two older commits. Settle re-check at +5 min: the summary comment was byte-identical (diff -q) and threads still 0. CI build SUCCESS on fce8613; MERGEABLE / CLEAN.

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 ConfigureAwait(false) guarantees a thread-pool hop, which it does not. Both fixes are XML doc comments only — git diff 0a9600f..fce8613 -- src/Daqifi.Core/ contains no non-comment line — so the bench result from the production commit still stands and was not re-run.

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 fce8613 as ready for review; I’m not taking any merge action.

@tylerkron
tylerkron added this pull request to the merge queue Aug 12, 2026
Merged via the queue into main with commit 7ef0282 Aug 12, 2026
1 check passed
@tylerkron
tylerkron deleted the fix/connect-path-configureawait-495 branch August 12, 2026 21:58
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.

bug(transport): sync Connect()/factory helpers deadlock under a SynchronizationContext — connect path lacks ConfigureAwait(false)

1 participant