Modernize to .NET 10, fix latent thread synchronization related Race conditions - #182
Open
Amir Burbea (amirburbea) wants to merge 38 commits into
Open
Amir Burbea (amirburbea) wants to merge 38 commits into
Amir Burbea (amirburbea) wants to merge 38 commits into
Conversation
2. Workaround to windows only thread affinity in Native32 for Linux 3. Solution updated to target Net7, Net6, Net472, and NetCoreapp3.1
- Upgraded MSTest packages with security vulerabilites - Reverted StyleCop package update from previous commit
- Target net8.0 across all projects, dropping net6.0/net7.0/net472 - Remove legacy .NET Standard packages (System.Linq.Expressions 4.3.0, Microsoft.CSharp 4.7.0, System.Diagnostics.*, etc.) now built into the runtime - Upgrade Microsoft.CodeAnalysis.Scripting 4.4.0 -> 5.3.0 - Upgrade MSTest 3.x -> 4.x, System.Reactive 5.x -> 6.x, and other packages - Fix ConfigModifier gate: replace Monitor (thread-affine) with SemaphoreSlim + AsyncLocal depth counter, making it safe for async tests that release from a different thread while remaining re-entrant for nested Modify() calls - Make T4 template import conditional so dotnet CLI builds work without VS - Add trill.runsettings with StopRunOnFirstFailure for faster test diagnosis - All 3185 tests pass on .NET 8
…er concurrent scheduling When both inputs complete concurrently, ProcessPendingBatches() uses Monitor.TryEnter and silently returns if another thread holds the lock. The subsequent Monitor.Enter block then called base.OnCompleted() immediately, propagating completion downstream before queued batches were processed. Fixed by calling ProcessPendingBatches() inside the Monitor.Enter block before base.OnCompleted() - Monitor is reentrant so TryEnter succeeds for the owning thread, guaranteeing all pending batches are flushed first.
…on tree compat - Target net10.0 across all projects - Remove StyleCop from core library (was never running on net8 due to netstandard2.0 condition) - Standardize test projects on StyleCop 1.1.118 - Suppress SA1137 in test ruleset (T4-generated files have non-standard indentation) - Fix AdHocTests: string.Join with params object[] instead of params ReadOnlySpan<object> to avoid CS8640/CS9226 in expression trees under .NET 10
Author
@microsoft-github-policy-service agree |
BinaryDecoder: - DecodeFloat, DecodeDouble, DecodeGuid, ReadIntFixed: replace new byte[] with stackalloc Span<byte>; add Span<byte> overload of ReadAllRequiredBytes - DecodeString: bypass DecodeByteArray, rent from ArrayPool<byte>, decode string, return buffer — avoids a heap allocation per string field BinaryEncoder: - Encode(float): replace BitConverter.GetBytes (allocates) with stackalloc + BitConverter.TryWriteBytes - Encode(Guid): replace ToByteArray() (allocates) with stackalloc + TryWriteBytes - Encode(string): replace Encoding.UTF8.GetBytes(string) (allocates) with ArrayPool<byte> rent, encode into span, delegate to Encode(ReadOnlySpan<byte>)
Amir Burbea (amirburbea)
force-pushed
the
modernize-net10
branch
from
March 24, 2026 05:03
3b61c17 to
b4e281f
Compare
The codegen cache (EquiJoinStreamable.cachedPipes) persisted across tests in the same process, causing JoinTestWithException to silently succeed (no exception thrown) when a compatible join had already been compiled by an earlier test. Fix: add internal Clear() to SafeConcurrentDictionary, expose cachedPipes as internal on EquiJoinStreamable, and clear the exact cache instance in [TestInitialize] before each run. The test now uses a named JoinResult struct so the closed generic type can be referenced at compile time, and asserts the StreamProcessingException is always thrown.
|
Great! Thanks |
Amir Burbea (amirburbea)
force-pushed
the
modernize-net10
branch
from
March 26, 2026 22:00
5a5a63f to
e86158a
Compare
…plemented with - instead of +. SimpleTesting does not have a test for this.
…tackalloc Begin to modernize codebase by replacing lambda expressions with method group references in ForEachAsync, Subscribe, and similar calls. Update collection initializations to use C# 12 collection expressions for improved readability. Use targeted new in several places. Remove StyleCop.Analyzers references from project files, and update Main method and array initializations in Program.cs. Checkpoint related version headers are now sent/received via stack allocated spans as opposed to byte[].
Configures Microsoft.StreamProcessing to pack as Trill.StreamProcessing, bundling both the core and Provider assemblies into a single package. Fixes broken version calculation in Directory.Build.props. Adds package README, license, and repository metadata pointing to the amirburbea/trill fork. Marks Microsoft.StreamProcessing.Provider as non-packable since it is bundled into the main package.
- Use targeted new - Use System.Threading.Lock - Use ArgumentNullException.ThrowIfNull/ArgumentOutOfRangeException.ThrowIf*
Made-with: Cursor
Made-with: Cursor
…g for a single retry after recompile.
Amir Burbea (amirburbea)
force-pushed
the
modernize-net10
branch
from
April 18, 2026 21:05
2821d40 to
dce5d14
Compare
…net pack on the csproj.
Amir Burbea (amirburbea)
force-pushed
the
modernize-net10
branch
from
April 18, 2026 21:11
dce5d14 to
0ab555a
Compare
Amir Burbea (amirburbea)
force-pushed
the
modernize-net10
branch
from
May 9, 2026 05:09
13c41da to
19cd1e9
Compare
CanRepresentAsColumnar incorrectly reported several payload shapes as
columnar-representable when Trill's columnar reconstitution (which assigns each
field/autoprop by plain assignment) cannot actually construct them:
- a get-only autoprop (no setter at all) was skipped entirely instead of
disqualifying the type
- an init-only setter (`{ get; init; }`) was accepted because SetMethod.IsPublic
is true for init same as for a normal setter - the IsExternalInit modifier was
never checked
- a public readonly instance field (as in a readonly struct, or a hand-written
immutable type) was never inspected at all; only non-public fields were scanned
All three previously fell through to "true" and then failed for real the first
time a query tried to generate the columnar batch, silently degrading to
row-based execution (or throwing, under DontFallBackToRowBasedExecution).
Every consumer of CanRepresentAsColumnar (ingress, Group, EquiJoin, Shuffle x3)
already treats it as a pre-filter for its own CanGenerateColumnar and falls back
to a row-based pipe on failure, so tightening it cannot regress a case that
works today - confirmed by the full SimpleTesting suite (3200 tests) passing
unchanged.
Also flips StreamProperties<TKey,TPayload>.IsColumnar from internal to a public
getter (setter stays internal) so a consumer can check whether a given payload
type actually got the columnar path without reflection.
Added CanRepresentAsColumnarTests covering both fixes plus the pre-existing
correctly-rejected shapes (no nullary ctor, non-public setter, manual backing
field, abstract type) as non-regression checks.
- Microsoft.CodeAnalysis.Scripting 5.3.0 -> 5.9.0 (Core + Provider) - several minor releases behind, including allocation-reduction work in Roslyn's general-purpose hot paths (e.g. GreenNode.To(Full)String, GetEscapedMetadataName) that the runtime codegen in this repo exercises on every columnar compile - System.Numerics.Tensors 10.0.0 -> 10.0.12 - System.Reactive 6.1.0 -> 7.0.0 (SimpleTesting, PerformanceTesting) - System.Reactive.Linq stays at 6.1.0, already latest - Microsoft.NET.Test.Sdk 18.3.0 -> 18.10.0, MSTest.TestAdapter/TestFramework 4.1.0 -> 4.4.0 (SimpleTesting) Verified: full solution builds clean, full SimpleTesting suite (3200 tests, including the CanRepresentAsColumnarTests from the previous commit) passes unchanged. Bump to 10.2.1.
System.Reactive.Linq stopped receiving releases at 6.1.0 - its contents were folded back into the main System.Reactive package starting with v7 (the split Core/Linq/Interfaces/PlatformServices packages were consolidated). Both test projects already reference System.Reactive 7.0.0, which already contains everything System.Reactive.Linq provided; the separate reference was dead weight. Test/perf projects only - the actual Microsoft.StreamProcessing package has no Rx dependency at all, so this doesn't touch anything already published as 10.2.1. Verified: full solution builds clean, full SimpleTesting suite (3200 tests) passes unchanged.
The get-only-autoprop fix in the previous commit (setMethod == null -> return
false, instead of continue) was correct for a genuine user-defined get-only
property, but every `record` (not `record struct`) also synthesizes a
compiler-generated, get-only, non-public `protected virtual Type
EqualityContract => typeof(T);` property. That property now tripped the same
check, so as of 10.2.1 EVERY record class - including a fully mutable one
with plain `{ get; set; }` properties - was incorrectly reported non-columnar.
Worse than the original bug, which accidentally tolerated this shape.
EqualityContract has no independent storage (it's always re-derived from the
runtime type), so there's nothing for columnar reconstitution to lose by not
touching it. The fix only skips the get-only case when the getter itself is
non-public - a genuinely non-public autoprop with a real setter (protected
get+set, say) still disqualifies the type exactly as before; only the
'non-public AND no setter at all' combination unique to EqualityContract is
exempted.
Added regression tests: a plain mutable record class must stay columnar
despite EqualityContract, and a class with a real non-public get+set autoprop
must still be rejected (proving the fix didn't over-broaden).
Verified: full solution builds clean, full SimpleTesting suite (3202 tests,
now including both new regression tests) passes.
Bump to 10.2.2. 10.2.1 should be treated as broken for any record class
payload and superseded by this version.
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
2 participants
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.
Using Adam Cohen (@adam-dot-cohen)'s PR as a base, update all libraries to net10.0, but remove the dotnet standard library references (i.e. Microsoft.CSharp).
Fixed locking in ConfigModifier to support re-entry
Fixed race condition in BinaryPipe
Use newer Assert methods in test libraries
@microsoft-github-policy-service agree