Conversation
There was a problem hiding this comment.
Pull request overview
Ports the fix for #3736 into release/6.1 so ExecuteScalar()/ExecuteScalarAsync() no longer silently ignores server-side errors that arrive after the first row/result, preventing transactional “zombie” scenarios.
Changes:
- Drain remaining result sets in
SqlCommand.CompleteExecuteScalar()(sync) and theExecuteScalarAsyncpath to force pending error tokens to surface. - Add new manual regression tests for
ExecuteScalar/ExecuteScalarAsync, including transactional rollback scenarios. - Update an existing multiple-results manual test to reflect the new error-propagation behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs | Ensures ExecuteScalar (sync/async) drains remaining results so deferred server errors are propagated. |
| src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs | Same ExecuteScalar draining behavior ported to the .NET Framework implementation. |
| src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlCommand/SqlCommandExecuteScalarTest.cs | Adds manual regression coverage for conversion-error-after-data and transaction rollback behavior. |
| src/Microsoft.Data.SqlClient/tests/ManualTests/ProviderAgnostic/MultipleResultsTest/MultipleResultsTest.cs | Updates ExecuteScalar test expectations now that ExecuteScalar drains and throws on later errors. |
| src/Microsoft.Data.SqlClient/tests/ManualTests/Microsoft.Data.SqlClient.ManualTesting.Tests.csproj | Includes the new manual test file in the test project. |
| // Arrange | ||
| // Insert valid VARCHAR values - '42-43' is a valid string, not an invalid number | ||
| DataTestUtility.CreateTable(connection, tableName, "(Id INT IDENTITY(1,1) NOT NULL, Val VARCHAR(10) NOT NULL)"); | ||
| using (SqlCommand insertCmd = connection.CreateCommand()) | ||
| { |
There was a problem hiding this comment.
These regression tests rely on SQL Server returning at least one row before hitting the conversion error (data token followed by error token). With the current table definition (heap without a clustered key), scan order isn't guaranteed and the conversion error can occur before any row is returned, which may not exercise the specific failure mode this PR targets. Consider adding a clustered primary key (e.g., on Id) so the scan order is deterministic (Id 1 row returned before the Id 2 row triggers error 245).
|
|
||
| // Drain remaining results to ensure all error tokens are processed | ||
| // before returning the result (fix for GH issue #3736). | ||
| while (await reader.NextResultAsync(cancellationToken).ConfigureAwait(false)) | ||
| { } |
There was a problem hiding this comment.
In this async path, draining remaining result sets uses the user-provided cancellationToken. If the token is canceled after the first ReadAsync completes, NextResultAsync will typically throw OperationCanceledException which is currently caught by the generic catch and reported via source.SetException(e), producing a faulted task rather than a canceled task. Consider handling OperationCanceledException/TaskCanceledException explicitly (or checking cancellationToken.IsCancellationRequested) and calling source.SetCanceled() to preserve ExecuteScalarAsync cancellation semantics.
| // Drain remaining results to ensure all error tokens are processed | ||
| // before returning the result (fix for GH issue #3736). | ||
| while (await reader.NextResultAsync(cancellationToken).ConfigureAwait(false)) | ||
| { } |
There was a problem hiding this comment.
In this async path, the added drain loop awaits NextResultAsync(cancellationToken). If cancellation is requested after the initial ReadAsync has completed, NextResultAsync can throw OperationCanceledException which is currently handled by the generic catch and surfaced via source.SetException(e), yielding a faulted task instead of a canceled task. Consider catching OperationCanceledException/TaskCanceledException (or checking cancellationToken.IsCancellationRequested) and calling source.SetCanceled() to keep ExecuteScalarAsync cancellation behavior consistent.
| ConcurrentQueue<string> messages = new ConcurrentQueue<string>(); | ||
|
|
||
| connection.InfoMessage += (object sender, SqlInfoMessageEventArgs args) => | ||
| messages.Enqueue(args.Message); | ||
|
|
||
| connection.Open(); | ||
|
|
||
| command.CommandText = s_sqlStatement; | ||
|
|
||
| // ExecuteScalar will select the first result set and the info message preceding it, then stop. | ||
| command.ExecuteScalar(); | ||
| Assert.True(messages.TryDequeue(out string lastMessage)); | ||
| Assert.Empty(messages); | ||
| Assert.Equal(ResultSet1_Message, lastMessage); | ||
| // ExecuteScalar now drains all result sets to ensure errors are not silently ignored (GH #3736 fix). | ||
| // Since the SQL statement contains RAISERRORs after the first result set, an exception is thrown. | ||
| SqlException ex = Assert.Throws<SqlException>(() => command.ExecuteScalar()); | ||
| Assert.Contains("Error 1", ex.Message); |
There was a problem hiding this comment.
messages and the InfoMessage event subscription are now unused in this test after switching to Assert.Throws for ExecuteScalar. Consider removing the queue and handler (or asserting on the collected messages) to avoid dead code and keep the test focused on the new behavior.
| try | ||
| { | ||
| // Arrange | ||
| // Insert valid VARCHAR values - '42-43' is a valid string, not an invalid number |
There was a problem hiding this comment.
... is a valid string, but not a valid number.
3 participants
Description
Port #3912 to release/6.1 to fix Execute Scalar issue #3736
Issues
Fixes #3736