[release/6.0-rc1] Fix #25225 by forcing usage of renting and populating commands when using relational command cache #25668
Conversation
…te split collection (only use cached command as template)
…Template to avoid confusion.
|
|
billybraga
changed the title
There was a problem hiding this comment.
This looks good to me, thanks @billybraga. We'll discuss later today about whether this can go into rc1.
Co-authored-by: Shay Rojansky <roji@roji.org>
Yeah, that was partially intentional; right now the command renting mechanism on RelationalConnection only "pools" one relational command. When using split query (on SQL Server), multiple relational commands are used in parallel, which means they wouldn't be cached anyway - only the main one would (FWIW this was also the original logic behind renting and populating for the main command but not for the others, which led to this bug). In any case, not returning a command to the connection isn't a bug - it's just a missed optimization at most; we can always improve this and implement a real pool with more than one commands for SQL Server. In any case I suggest we keep this separate from this bug, which is important to fix ASAP - I'll open a tracking issue for the rest. |
ajcvickers
changed the title
|
Merging this into main now, will port fix to release branches. |
…ng commands when using relational command cache (#25668) * Fix #25225 by avoiding reusing the relational command to populate split collection (only use cached command as template) * Extract the cached part of IRelationalCommand into IRelationalCommandTemplate to avoid confusion. * Apply suggestions from code review Co-authored-by: Shay Rojansky <roji@roji.org> Co-authored-by: Billy Braga <bbraga@progi.com> Co-authored-by: Shay Rojansky <roji@roji.org>
Fixes #25225.
Description
As part of the query performance work in EF Core 6.0, a concurrency state bug was accidentally introduced where the same stateful object (RelationalCommand) may get reused concurrently, leading to issues.
Customer Impact
Running the same query concurrently as a split query on SQL Server may fail in unpredictable ways.
How found
User report.
Test coverage
Included in this PR.
Regression?
Yes, as result of a performance optimization introduced in EF Core 6.0.
Risk
Very low, the PR simply duplicates the stateful RelationalCommand for the non-primary split query case, a technique already used in other scenarios (e.g. single query).
There is also a remaining possible issue that I see:RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.PopulateSplitIncludeCollection(...)(andPopulateSplitIncludeCollectionAsync) don't return their command template usingConnection.ReturnCommand(...)like the other places where commands are rented.