[6.1] Port #3912 to release/6.1#4133
Merged
paulmedynski merged 2 commits intodev/samsharma2700/backport_3912_61from Apr 2, 2026
Merged
[6.1] Port #3912 to release/6.1#4133paulmedynski merged 2 commits intodev/samsharma2700/backport_3912_61from
paulmedynski merged 2 commits intodev/samsharma2700/backport_3912_61from
Conversation
…code removal Agent-Logs-Url: https://github.com/dotnet/SqlClient/sessions/9644d850-f130-470d-b04a-74c197d5d7cc Co-authored-by: paulmedynski <[email protected]>
Copilot
AI
changed the title
[WIP] Port #3912 to release/6.1 to fix Execute Scalar issue
[6.1] Port #3912 to release/6.1
Apr 2, 2026
paulmedynski
approved these changes
Apr 2, 2026
paulmedynski
merged commit 113aab3
into
dev/samsharma2700/backport_3912_61
4 checks passed
Contributor
There was a problem hiding this comment.
Pull request overview
Ports the GH #3736 fix to release/6.1, ensuring ExecuteScalar/ExecuteScalarAsync properly surface server errors that occur after the first result, and incorporates review-driven improvements for cancellation behavior and test reliability.
Changes:
- Updated
InternalExecuteScalarAsync(netcore + netfx) to treatOperationCanceledExceptionas task cancellation (rather than faulting) when the provided cancellation token is canceled. - Made
ExecuteScalarregression ManualTests deterministic by addingPRIMARY KEY CLUSTEREDto identity columns used by the test tables and adjusted related comments. - Removed unused
InfoMessage/ConcurrentQueueplumbing fromMultipleResultsTest.ExecuteScalar().
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlCommand/SqlCommandExecuteScalarTest.cs | Makes regression test table scans more deterministic via clustered PK and updates comments. |
| src/Microsoft.Data.SqlClient/tests/ManualTests/ProviderAgnostic/MultipleResultsTest/MultipleResultsTest.cs | Removes unused message-collection code from the ExecuteScalar test. |
| src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs | Ensures canceled ExecuteScalarAsync completes as canceled (not faulted) in the drain-loop cleanup path. |
| src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs | Same cancellation semantics adjustment as netfx for ExecuteScalarAsync. |
| // sourceTable.Val is VARCHAR - both '12345' and '42-43' are valid strings | ||
| DataTestUtility.CreateTable(connection, sourceTable, "(Id INT IDENTITY(1,1) NOT NULL, Val VARCHAR(10) NOT NULL)"); | ||
| DataTestUtility.CreateTable(connection, targetTable, "(Id INT IDENTITY(1,1) NOT NULL, Val1 INT NOT NULL, Val2 INT NOT NULL)"); | ||
| // sourceTable.Val is VARCHAR - both '12345' and '42-43' are valid strings but not valid numbers |
There was a problem hiding this comment.
The comment is inaccurate: '12345' is a valid number when converting VARCHAR to INT; only '42-43' fails conversion. Consider rewording to avoid implying both values are not valid numbers (e.g., clarify that one value converts and the other does not).
Suggested change
| // sourceTable.Val is VARCHAR - both '12345' and '42-43' are valid strings but not valid numbers | |
| // sourceTable.Val is VARCHAR - both '12345' and '42-43' are valid strings, but only '12345' converts to INT |
| // sourceTable.Val is VARCHAR - both '12345' and '42-43' are valid strings | ||
| DataTestUtility.CreateTable(connection, sourceTable, "(Id INT IDENTITY(1,1) NOT NULL, Val VARCHAR(10) NOT NULL)"); | ||
| DataTestUtility.CreateTable(connection, targetTable, "(Id INT IDENTITY(1,1) NOT NULL, Val1 INT NOT NULL, Val2 INT NOT NULL)"); | ||
| // sourceTable.Val is VARCHAR - both '12345' and '42-43' are valid strings but not valid numbers |
There was a problem hiding this comment.
The comment is inaccurate: '12345' converts to INT successfully; the conversion error is caused by the '42-43' row. Rewording this will prevent confusion when interpreting why the WHERE clause throws.
Suggested change
| // sourceTable.Val is VARCHAR - both '12345' and '42-43' are valid strings but not valid numbers | |
| // sourceTable.Val is VARCHAR - both '12345' and '42-43' are valid strings, but only '42-43' cannot be converted to INT |
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
Ports the fix for #3736 (
ExecuteScalar/ExecuteScalarAsyncsilently discarding server errors after the first result) intorelease/6.1, and addresses review feedback on the initial port.ExecuteScalarAsynccancellation semantics (netcore+netfx):catchblock now distinguishesOperationCanceledExceptionfrom other exceptions, callingsource.SetCanceled()instead ofsource.SetException(e)so a canceledExecuteScalarAsyncproduces a properly canceled task rather than a faulted one.Regression test reliability (
SqlCommandExecuteScalarTest.cs):IdasPRIMARY KEY CLUSTERED, ensuring SQL Server scans rows in insertion order (Id=1 before Id=2). Without this, a heap scan could encounter the conversion error before returning any row, bypassing the specific code path under test.Dead code removal (
MultipleResultsTest.cs):ConcurrentQueue<string> messagesandInfoMessagehandler fromExecuteScalar()after the test was updated to useAssert.Throws<SqlException>.Issues
Testing
ExecuteScalar_ShouldThrowOnConversionError,ExecuteScalarAsync_ShouldThrowOnConversionError, and their transaction variants) now run against tables with a clustered primary key, making scan order deterministic and the tests reliable.Guidelines
Please review the contribution guidelines before submitting a pull request: