FileStream and PipeStream code reuse for async File IO on Windows #81960

adamsitnik · 2023-02-10T15:59:14Z

No description provided.

…SafePipeHandle

…y Pipes

…PipeStream in case of pipes

…nceledException when user obtains handle and calls CancelIoEx (this is what ReadAsync and WriteAsync do)

ghost · 2023-02-10T15:59:25Z

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author:	adamsitnik
Assignees:	-
Labels:	`area-System.IO`
Milestone:	-

adamsitnik · 2023-02-10T17:32:24Z

src/libraries/Common/src/Microsoft/Win32/SafeHandles/OverlappedValueTaskSource.Windows.cs

+                    // For invalid handles, detect the error and mark our handle
+                    // as invalid to give slightly better error messages.  Also
+                    // help ensure we avoid handle recycling bugs.
+                    _fileHandle!.SetHandleAsInvalid();


in case Dispose happened after the operation was started, only PipeStream was so far disposing the handle:

runtime/src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.Windows.cs

Line 641 in 84c2ff1

_handle!.SetHandleAsInvalid();

RandomAccess was doing that only when the handle was disposed before the operation was started:

runtime/src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs

Line 272 in 84c2ff1

errorCode = FileStreamHelpers.GetLastWin32ErrorAndDisposeHandleIfInvalid(handle);

adamsitnik · 2023-02-10T17:36:20Z

src/libraries/Common/src/Microsoft/Win32/SafeHandles/OverlappedValueTaskSource.Windows.cs

+            _threadPoolBinding = threadPoolBinding;
+            _canSeek = canSeek;
+            _source.RunContinuationsAsynchronously = true;
+            _preAllocatedOverlapped = PreAllocatedOverlapped.UnsafeCreate(s_ioCallback, this, null);


so far SafeFileHandle was using the UnsafeCreate (not touching the execution context):

runtime/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.OverlappedValueTaskSource.Windows.cs

Line 66 in 5a62a79

_preallocatedOverlapped = PreAllocatedOverlapped.UnsafeCreate(s_ioCallback, this, null);

while PipeStream used the "safe" ctor:

runtime/src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.ValueTaskSource.cs

Line 34 in 5a62a79

_preallocatedOverlapped = new PreAllocatedOverlapped(s_ioCallback, this, null);

We should unify that and I am not sure which option to choose. If the safe ctor should be the default we need to add tests for this scenario (currently all are passing)

Do we run any user code as part of s_ioCallback? I believe the answer is "no", in which case it can be UnsafeCreate.

adamsitnik · 2023-02-10T17:40:03Z

src/libraries/Common/src/Microsoft/Win32/SafeHandles/OverlappedValueTaskSource.Windows.cs

+            {
+                case Interop.Errors.ERROR_SUCCESS:
+                case Interop.Errors.ERROR_PIPE_CONNECTED: // special case for when the client has already connected (used only by ConnectionValueTaskSource)
+                case Interop.Errors.ERROR_BROKEN_PIPE when _isRead: // write to broken pipe should throw, read should return 0


special handling these failure for only the read operations was so far done only by the pipes:

runtime/src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.ValueTaskSource.cs

Lines 167 to 176 in 5a62a79

if (!_isWrite)

{

bool messageCompletion = true;

switch (errorCode)

{

case Interop.Errors.ERROR_BROKEN_PIPE:

case Interop.Errors.ERROR_PIPE_NOT_CONNECTED:

case Interop.Errors.ERROR_NO_DATA:

errorCode = 0;

I've missed it in #77543

adamsitnik · 2023-02-10T17:43:06Z

src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.ValueTaskSource.cs

+                    case Interop.Errors.ERROR_NO_DATA:
+                    // The handle was closed
+                    case Interop.Errors.ERROR_INVALID_HANDLE:
+                        _pipeStream.State = PipeState.Broken;


the need of updating this state is one of the reasons why relaxing RandomAccess requirements to support non-seekable files was no the way to go to share the code between FileStream and PipeStream. We could ofc just await the operations and call last error, but it would not be performant.

the need of updating this state is one of the reasons why relaxing RandomAccess requirements to support non-seekable files was no the way to go to share the code between FileStream and PipeStream

Can you elaborate? I'd really like to get to a place where PipeStream doesn't need its own implementation and can just use RandomAccess.

For some errors, PipeStream modifies it's state before throwing an exception or returning result. This state is publicly exposed (IsMessageComplete for example).

runtime/src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.ValueTaskSource.cs

Lines 165 to 185 in 5a89bbe

private protected override void CompleteCore(uint errorCode, uint numBytes)

{

if (!_isWrite)

{

bool messageCompletion = true;

switch (errorCode)

{

case Interop.Errors.ERROR_BROKEN_PIPE:

case Interop.Errors.ERROR_PIPE_NOT_CONNECTED:

case Interop.Errors.ERROR_NO_DATA:

errorCode = 0;

break;

case Interop.Errors.ERROR_MORE_DATA:

errorCode = 0;

messageCompletion = false;

break;

}

_pipeStream.UpdateMessageCompletion(messageCompletion);

In case of IsMessageComplete we would need to have sth like this:

public class PipeStream { public ValueTask<int> ReadAsync(Buffer, CancellationToken) { int result = await RandomAccess.ReadAsync(!Handle, Buffer, CancellationToken); if (LastError() == ERROR_MORE_DATA) { UpdateMessageCompletion(messageCompletion) } return result; } }

Another problem is that SafePipeStream binds a handle to the thread pool and currently there are no public APIs that would allow for creating SafeFileHandle out of SafePipeHandle.

Most likely both SafePipeHandle and SafeFileHandle would need to expose ThreadPoolBoundHandle properties (Windows-only, low level, easy to mess up things):

runtime/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs

Line 31 in 5a89bbe

internal ThreadPoolBoundHandle? ThreadPoolBinding { get; set; }

runtime/src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.Windows.cs

Lines 14 to 17 in 5a89bbe

public abstract partial class PipeStream : Stream

{

internal const bool CheckOperationsRequiresSetHandle = true;

internal ThreadPoolBoundHandle? _threadPoolBinding;

and add ctors that accept a handle and ThreadPoolBoundHandle

adamsitnik · 2023-02-10T17:44:29Z

src/libraries/System.IO.Pipes/tests/PipeStreamConformanceTests.cs

@@ -278,7 +278,7 @@ public async Task CancelTokenOn_ServerWaitForConnectionAsyncWithOuterCancellatio
            Task waitForConnectionTask = server.WaitForConnectionAsync(cts.Token);

            Assert.True(InteropTest.CancelIoEx(server.SafePipeHandle), "Outer cancellation failed");
-            await Assert.ThrowsAsync<IOException>(() => waitForConnectionTask);
+            await Assert.ThrowsAsync<OperationCanceledException>(() => waitForConnectionTask);


in theory this is a breaking change. I wanted to align WaitForConnectionAsync with ReadAsync and WriteAsync.

in theory this is a breaking change

We've gone to some lengths to maintain this behavior from .NET Framework. If we want to take a breaking change around it, I'm ok with that, but I don't have a great sense of how impactful that will be.

I don't have a great sense of how impactful that will be

I hope that calling CancelIoEx rather than passing cancellable token to WaitForConnectionAsync is rare. I don't have strong opinion about it.

adamsitnik · 2023-02-10T17:45:06Z

src/libraries/System.IO.Pipes/tests/PipeStreamConformanceTests.cs

@@ -502,8 +502,6 @@ public async Task ReadAsync_DisconnectDuringRead_Returns0()
        }

        [PlatformSpecific(TestPlatforms.Windows)] // Unix named pipes are on sockets, where small writes with an empty buffer will succeed immediately
-        [SkipOnPlatform(TestPlatforms.LinuxBionic, "SElinux blocks UNIX sockets in our CI environment")]


this was just a copy paste error (the test is marked as [PlatformSpecific(TestPlatforms.Windows)] so we don't need to skip it on other platforms)

adamsitnik · 2023-02-10T17:47:25Z

/azp list

adamsitnik · 2023-02-10T17:48:33Z

/azp run runtime-libraries-coreclr outerloop-windows

azure-pipelines · 2023-02-10T17:48:45Z

Azure Pipelines successfully started running 1 pipeline(s).

jeffhandley · 2023-03-27T18:51:34Z

@adamsitnik I'm going to close this PR since it's gone stale and we're not actively driving it forward. We can resume this effort later in the release cycle if/as time permits.

adamsitnik added 7 commits February 10, 2023 08:55

reverse the nesting of SafeFileHandle.OverlappedValueTaskSource

ed96f09

make OverlappedValueTaskSource more generic so it could be reused by …

e81f16a

…SafePipeHandle

move some methods out of RandomAccess.Windows so they can be reused b…

27113c0

…y Pipes

move the files to Common

ffb0349

reuse the code in System.IO.Pipes

ea2517a

unify error handling for FileStream to behave exactly as it does for …

8a2cffc

…PipeStream in case of pipes

NamedPipeServerStream.WaitForConnectionAsync should throw OperationCa…

e97db33

…nceledException when user obtains handle and calls CancelIoEx (this is what ReadAsync and WriteAsync do)

adamsitnik added the area-System.IO label Feb 10, 2023

ghost assigned adamsitnik Feb 10, 2023

adamsitnik force-pushed the codeReuse branch from 5bbfd49 to 6d74c6a Compare February 10, 2023 17:19

cleanup after reading the code again on GH

e76bd6e

adamsitnik force-pushed the codeReuse branch from 6d74c6a to e76bd6e Compare February 10, 2023 17:24

adamsitnik commented Feb 10, 2023

View reviewed changes

This comment was marked as resolved.

Sign in to view

adamsitnik marked this pull request as ready for review February 10, 2023 17:48

adamsitnik requested a review from stephentoub February 10, 2023 17:49

build-analysis bot mentioned this pull request Feb 10, 2023

Test failure Loader\\classloader\\DictionaryExpansion\\DictionaryExpansion\\DictionaryExpansion.cmd #75244

Closed

jeffhandley closed this Mar 27, 2023

ghost locked as resolved and limited conversation to collaborators Apr 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FileStream and PipeStream code reuse for async File IO on Windows #81960

FileStream and PipeStream code reuse for async File IO on Windows #81960

adamsitnik commented Feb 10, 2023

ghost commented Feb 10, 2023

adamsitnik Feb 10, 2023

adamsitnik Feb 10, 2023

stephentoub Feb 10, 2023

adamsitnik Feb 10, 2023

adamsitnik Feb 10, 2023

stephentoub Feb 10, 2023

adamsitnik Feb 10, 2023

adamsitnik Feb 10, 2023

adamsitnik Feb 10, 2023

stephentoub Feb 10, 2023

adamsitnik Feb 10, 2023 •

edited

Loading

adamsitnik Feb 10, 2023

adamsitnik commented Feb 10, 2023

This comment was marked as resolved.

adamsitnik commented Feb 10, 2023

azure-pipelines bot commented Feb 10, 2023

jeffhandley commented Mar 27, 2023

	if (!_isWrite)
	{
	bool messageCompletion = true;

	switch (errorCode)
	{
	case Interop.Errors.ERROR_BROKEN_PIPE:
	case Interop.Errors.ERROR_PIPE_NOT_CONNECTED:
	case Interop.Errors.ERROR_NO_DATA:
	errorCode = 0;

	private protected override void CompleteCore(uint errorCode, uint numBytes)
	{
	if (!_isWrite)
	{
	bool messageCompletion = true;

	switch (errorCode)
	{
	case Interop.Errors.ERROR_BROKEN_PIPE:
	case Interop.Errors.ERROR_PIPE_NOT_CONNECTED:
	case Interop.Errors.ERROR_NO_DATA:
	errorCode = 0;
	break;

	case Interop.Errors.ERROR_MORE_DATA:
	errorCode = 0;
	messageCompletion = false;
	break;
	}

	_pipeStream.UpdateMessageCompletion(messageCompletion);

	public abstract partial class PipeStream : Stream
	{
	internal const bool CheckOperationsRequiresSetHandle = true;
	internal ThreadPoolBoundHandle? _threadPoolBinding;

FileStream and PipeStream code reuse for async File IO on Windows #81960

FileStream and PipeStream code reuse for async File IO on Windows #81960

Conversation

adamsitnik commented Feb 10, 2023

ghost commented Feb 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsitnik Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsitnik commented Feb 10, 2023

This comment was marked as resolved.

adamsitnik commented Feb 10, 2023

azure-pipelines bot commented Feb 10, 2023

jeffhandley commented Mar 27, 2023

adamsitnik Feb 10, 2023 •

edited

Loading