Repository navigation
Conversation
…g source The NET8_0_OR_GREATER fast path reads TryGetNonEnumeratedCount, then copies into a T[count] buffer. The count is a live Count read (e.g. a ConcurrentQueue<T> a producer enqueues to), so it is not atomic with the foreach below: a source that grows after the count check indexes past the buffer with IndexOutOfRangeException, and one that shrinks returns an array padded with default(T) at the tail instead of its actual length. Found via a failing fork CI run of a managed-session reconnect test (ManagedSessionReconnectHarness.Requests snapshotting a ConcurrentQueue<IServiceRequest> shared with the scripted transport). Grow the buffer instead of indexing past it, and trim it if the source enumerated shorter than its reported count. Reproduced deterministically with an ICollection<T> whose Count does not match its enumeration (TryGetNonEnumeratedCount only takes the fast path for ICollection<T>, not the read-only interface).
…double ConcurrentQueue<T> implements the non-generic ICollection and IReadOnlyCollection<T>, not the generic ICollection<T>. The double implemented ICollection<T> instead, which made the net48/net472 LINQ fallback (Enumerable.ToArray, used outside NET8_0_OR_GREATER) call its CopyTo(T[], int) and throw, failing the test for the wrong reason on those frameworks (confirmed on the fork CI's windows-net48 shard).
|
|
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The fix and regression tests fully address the stale-count scenarios.
Review effort: Lite
Findings: None
What changed in this PR
Fixes ArrayOf.ToArrayOf(IEnumerable<T>) handling when a source’s reported count differs from its enumeration.
Changes:
- Dynamically grows buffers during enumeration.
- Trims buffers to the actual item count.
- Adds regression tests for growth and shrink scenarios.
| File | Description |
|---|---|
tests/Opc.Ua.Types.Tests/BuiltIn/ArrayOfTests.cs |
Adds stale-count regression coverage. |
src/Opc.Ua.Types/BuiltIn/ArrayOf.cs |
Corrects buffer sizing and trimming. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Author
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.
Description
The NET8_0_OR_GREATER fast path of
ToArrayOf<T>(IEnumerable<T>)sizes its buffer fromTryGetNonEnumeratedCountand then fills it with aforeach. The count read and the enumerationare not atomic, so a source that changes size between them — a
ConcurrentQueue<T>a producer isstill enqueuing to is the concrete case here — makes the copy overrun the buffer
(
IndexOutOfRangeException) or, if the source shrank, return an array padded withdefault(T).Found through a fork CI failure of
SlowActivationRecoveryBelowDeadlineAvoidsOuterPolicyAsync,which reads
ManagedSessionReconnectHarness.Requests— aConcurrentQueue<IServiceRequest>.ToArrayOf()— while a scripted transport on another thread is still enqueuing.
Changes:
ToArrayOf<T>(IEnumerable<T>?): grow the buffer (doubling, minimum 4) instead of indexing pastit, and trim it to the actual enumerated length before returning.
ConcurrentQueue<T>'s own interfaces (ICollection+IReadOnlyCollection<T>) whoseCountdisagrees with its enumeration, which is exactly what
TryGetNonEnumeratedCounttakes at facevalue. Both fail on master (one throws, one returns wrong data).
Related Issues
Checklist
Test evidence:
dotnet buildon all seven target frameworks: 0 warnings, 0 errors.Opc.Ua.Types.Testssuite (10 549 tests) green,including the two new deterministic reproductions.
master(one withIndexOutOfRangeException, one with awrong-length result) and pass on this branch.
dotnet format whitespace|style|analyzers --verify-no-changeson every changed file: no newdiagnostic beyond what is already present on unmodified
master.green, 149/149 jobs — https://github.com/biancode/UA-.NETStandard/actions/runs/36345671141
Full disclosure, AI tooling (Claude Code) was used to investigate this issue, implement the fix and tests, and validate the build/format/tests locally and on the fork CI before opening this PR.
🤖 Generated with Claude Code