ArgumentNullException when reading empty/null PLP strings with new async behavior#3884
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a test case to verify that reading empty and null PLP (Partially Length-Prefixed) strings asynchronously does not throw an ArgumentNullException when using the new async behavior. The test is a regression test for issue #593, which originally reported severe performance problems when reading large binary data asynchronously.
Changes:
- Added a new async test
ReadEmptyAndNullPlpStringsAsyncWithNewBehaviorthat validates reading NULL and empty string values from VARCHAR(MAX), NVARCHAR(MAX), and TEXT columns - The test disables compatibility mode to use the new async behavior via the UseCompatibilityAsyncBehaviour switch
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs
Outdated
Show resolved
Hide resolved
cheenamalhotra
added
the
Area\Tests
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@cheenamalhotra where should i see the PRs i request from copilot? |
|
@SimonCropp I doubt you can control Copilot in this repo - only in your own fork |
|
@ErikEJ so isnt that a bug in copilot, that it is offering to submit PRs that it can actually do? |
|
If you get a test that works quickly and reliably enough for CI use do you want me to include it in #3872 or do you want to include my change in this PR?, either is fine. |
…on-when-reading-empty/null-PLP-strings-with-new-async-behavior
| cmd.CommandText = $@" | ||
| CREATE TABLE {tableName} ( | ||
| Id INT PRIMARY KEY, | ||
| VarcharMaxCol VARCHAR(MAX), | ||
| NVarcharMaxCol NVARCHAR(MAX), | ||
| TextCol TEXT | ||
| )"; |
There was a problem hiding this comment.
Table name is injected into CREATE/INSERT/SELECT without quoting (e.g., CREATE TABLE {tableName}), while this file commonly wraps generated object names in brackets (e.g., create table [{tempTableName}]). Please bracket/quote the identifier consistently (and in all three statements) to avoid failures if the generated name contains characters that require quoting or collides with reserved words.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| // Create test table with PLP string columns | ||
| using (SqlCommand cmd = connection.CreateCommand()) | ||
| { | ||
| cmd.CommandText = $@" | ||
| CREATE TABLE {tableName} ( | ||
| Id INT PRIMARY KEY, | ||
| VarcharMaxCol VARCHAR(MAX), | ||
| NVarcharMaxCol NVARCHAR(MAX), | ||
| TextCol TEXT |
There was a problem hiding this comment.
The comment says these are “PLP string columns”, but TEXT is a deprecated LOB type and is not a PLP type like VARCHAR(MAX)/NVARCHAR(MAX). Either update the comment to reflect the actual types being tested, or change/remove the TEXT column to keep the test focused on PLP behavior.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
attempt to replicate #593 (comment)