-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Remove isAsync flag validation from FileStream constructors #123761
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
adamsitnik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot please address my feedback
...aries/System.Runtime/tests/System.IO.FileSystem.Tests/FileStream/ctor_sfh_fa_buffer_async.cs
Outdated
Show resolved
Hide resolved
...aries/System.Runtime/tests/System.IO.FileSystem.Tests/FileStream/ctor_sfh_fa_buffer_async.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
…ification Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
adamsitnik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (assuming the tests are going to pass).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Removes validation that the isAsync constructor argument must match SafeFileHandle.IsAsync, and updates tests accordingly so FileStream construction uses the handle’s async capability rather than the caller-provided flag.
Changes:
- Updated
FileStreamconstructors to ignore theisAsyncargument and consistently usehandle.IsAsyncwhen selecting the implementation strategy. - Removed now-unused throw helpers and resource strings related to async/sync handle mismatch validation.
- Updated the relevant
FileStreamconstructor test to assert mismatch is allowed and to exercise async read/write behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/libraries/System.Runtime/tests/System.IO.FileSystem.Tests/FileStream/ctor_sfh_fa_buffer_async.cs | Updates the mismatch test to be a theory and verifies behavior/operations when isAsync doesn’t match. |
| src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs | Removes unused throw helpers for async/sync handle mismatch. |
| src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs | Removes isAsync validation and switches strategy selection to use handle.IsAsync. |
| src/libraries/System.Private.CoreLib/src/Resources/Strings.resx | Removes unused resource strings for mismatch validation errors. |
...aries/System.Runtime/tests/System.IO.FileSystem.Tests/FileStream/ctor_sfh_fa_buffer_async.cs
Show resolved
Hide resolved
...aries/System.Runtime/tests/System.IO.FileSystem.Tests/FileStream/ctor_sfh_fa_buffer_async.cs
Show resolved
Hide resolved
...aries/System.Runtime/tests/System.IO.FileSystem.Tests/FileStream/ctor_sfh_fa_buffer_async.cs
Show resolved
Hide resolved
|
/ba-g infrastructure timeouts |
Description
FileStream constructors validated that the user-provided
isAsyncparameter matchedSafeFileHandle.IsAsync, throwingArgumentExceptionon mismatch. This validation is removed—constructors now ignore the parameter and always use the handle's actual async state.Changes
handle.IsAsyncto strategy selection, regardless of user-providedisAsyncparameterValidateHandleoverload that checked isAsync mismatchThrowArgumentException_HandleNotAsync/Sync) and resource strings (Arg_HandleNotAsync/Sync)FileStream.IsAsyncreflects handle state, not the ignored parameterUnmatchedAsyncIsAllowedtest to a Theory withInlineDatafor both true and false valuesExample
The
isAsyncparameter becomes effectively unused but remains for API compatibility.Original prompt
FileStreamctors #123760💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.