Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Jan 29, 2026

Description

FileStream constructors validated that the user-provided isAsync parameter matched SafeFileHandle.IsAsync, throwing ArgumentException on mismatch. This validation is removed—constructors now ignore the parameter and always use the handle's actual async state.

Changes

  • FileStream constructors: Always pass handle.IsAsync to strategy selection, regardless of user-provided isAsync parameter
  • Validation: Removed ValidateHandle overload that checked isAsync mismatch
  • Cleanup: Removed unused throw helpers (ThrowArgumentException_HandleNotAsync/Sync) and resource strings (Arg_HandleNotAsync/Sync)
  • Tests: Updated to verify FileStream.IsAsync reflects handle state, not the ignored parameter
    • Converted UnmatchedAsyncIsAllowed test to a Theory with InlineData for both true and false values
    • Added comprehensive async I/O verification (write, seek, read with content verification) to ensure the FileStream functions correctly with the handle's actual async state

Example

// Previously threw ArgumentException
var asyncHandle = new SafeFileHandle(..., isAsync: true);
var fs = new FileStream(asyncHandle, FileAccess.ReadWrite, 4096, isAsync: false);

// Now succeeds - fs.IsAsync == true (from handle, not parameter)

The isAsync parameter becomes effectively unused but remains for API compatibility.

Original prompt

This section details on the original issue you should resolve

<issue_title>Remove isAsync flag validation from FileStream ctors</issue_title>
<issue_description>For the historical reasons described here, .NET was so far throwing if the value of isAsync boolean flag did not match the value returned by SafeFileHandle.IsAsync.

We have agreed to remove these checks:

if (isAsync && !handle.IsAsync)
{
ThrowHelper.ThrowArgumentException_HandleNotAsync(nameof(handle));
}
else if (!isAsync && handle.IsAsync)
{
ThrowHelper.ThrowArgumentException_HandleNotSync(nameof(handle));
}

And simply pass the actual value of handle.IsAsync to the layer that chooses the right strategy, just like we do when the flag was not provided by the user:

_strategy = FileStreamHelpers.ChooseStrategy(this, handle, access, bufferSize, handle.IsAsync);

So the following lines need to be updated:

_strategy = FileStreamHelpers.ChooseStrategy(this, handle, access, bufferSize, isAsync);

_strategy = FileStreamHelpers.ChooseStrategy(this, safeHandle, access, bufferSize, isAsync);

And all unused code (like the throw helpers) needs to be removed as well. The tests also need to be updated accordingly.</issue_description>

Comments on the Issue (you are @copilot in this section)


💡 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.

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Copy link
Member

@adamsitnik adamsitnik left a 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

Copilot AI and others added 2 commits January 29, 2026 17:55
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Copilot AI changed the title [WIP] Remove isAsync flag validation from FileStream constructors Remove isAsync flag validation from FileStream constructors Jan 29, 2026
Copilot AI requested a review from adamsitnik January 29, 2026 18:00
…ification

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
@adamsitnik adamsitnik marked this pull request as ready for review January 29, 2026 18:10
Copilot AI review requested due to automatic review settings January 29, 2026 18:10
Copy link
Member

@adamsitnik adamsitnik left a 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).

Copy link
Contributor

Copilot AI left a 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 FileStream constructors to ignore the isAsync argument and consistently use handle.IsAsync when selecting the implementation strategy.
  • Removed now-unused throw helpers and resource strings related to async/sync handle mismatch validation.
  • Updated the relevant FileStream constructor 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove isAsync flag validation from FileStream ctors

3 participants