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
| using (var switchHelper = new SwitchesHelper()) | ||
| { | ||
| originalSwitchValue = switchHelper.UseCompatibilityAsyncBehaviour; | ||
| switchHelper.UseCompatibilityAsyncBehaviour = false; | ||
| } |
There was a problem hiding this comment.
The SwitchesHelper usage pattern is incorrect. The helper is being disposed at line 1102 before the actual test runs, which means the UseCompatibilityAsyncBehaviour switch will be restored to its original value before the test executes.
The correct pattern is to keep the SwitchesHelper alive for the entire scope where you need the modified switch value. The helper should be created at the beginning of the try block and only disposed after all test operations that depend on the switch value are complete.
Looking at other tests in this file (e.g., CheckNullRowVersionIsDBNull at line 270), the pattern should be:
- Create the SwitchesHelper at the start of the try block
- Set the switch value
- Run all test operations
- The helper will be automatically disposed at the end of the using block, restoring the original value
Additionally, lines 1185-1188 attempt to restore the switch value manually, but this is unnecessary and incorrect since the Dispose method of SwitchesHelper already restores all original values automatically.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| if (originalSwitchValue.HasValue) | ||
| { | ||
| using (var switchHelper = new SwitchesHelper()) | ||
| { | ||
| switchHelper.UseCompatibilityAsyncBehaviour = originalSwitchValue; | ||
| } | ||
| } |
There was a problem hiding this comment.
The manual restoration of the switch value is unnecessary and incorrect. The SwitchesHelper class automatically restores all original switch values in its Dispose method. This entire block (lines 1183-1189) should be removed, as it serves no purpose and is based on a misunderstanding of how the helper works.
Furthermore, the variable 'originalSwitchValue' at line 1093 is only used in this incorrect restoration logic and can also be removed.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
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. |
4 participants
attempt to replicate #593 (comment)