Skip to content
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

Add clarification to AnonymousPipeClientStream that async is not supported on Windows #7723

Merged
merged 2 commits into from
Feb 15, 2022

Conversation

daveaglick
Copy link
Contributor

Summary

This came up recently in a project of mine: daveaglick/MsBuildPipeLogger#4. It appears that on Windows platforms "asynchronous (overlapped) read and write operations are not supported by anonymous pipes" (from https://docs.microsoft.com/en-us/windows/win32/ipc/anonymous-pipe-operations). This limitation was also encountered by a developer and filed in the dotnet/runtime repository: dotnet/runtime#23638.

I'm suggesting some slight language changes that make this behavior clearer in the .NET docs for AnonymousPipeClientStream. It's possible that similar changes may be needed for AnonymousPipeServerStream docs as well. It's also possible that I've misunderstood the discussions in the linked issues (I'm certainly not an expert on pipes or the Win32 APIs) and more clarification or specifics are needed, but hopefully this gets the ball rolling.

@daveaglick daveaglick requested a review from a team as a code owner February 15, 2022 13:47
@ghost
Copy link

ghost commented Feb 15, 2022

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

Issue Details

Summary

This came up recently in a project of mine: daveaglick/MsBuildPipeLogger#4. It appears that on Windows platforms "asynchronous (overlapped) read and write operations are not supported by anonymous pipes" (from https://docs.microsoft.com/en-us/windows/win32/ipc/anonymous-pipe-operations). This limitation was also encountered by a developer and filed in the dotnet/runtime repository: dotnet/runtime#23638.

I'm suggesting some slight language changes that make this behavior clearer in the .NET docs for AnonymousPipeClientStream. It's possible that similar changes may be needed for AnonymousPipeServerStream docs as well. It's also possible that I've misunderstood the discussions in the linked issues (I'm certainly not an expert on pipes or the Win32 APIs) and more clarification or specifics are needed, but hopefully this gets the ball rolling.

Author: daveaglick
Assignees: -
Labels:

area-System.IO

Milestone: -

@opbld33
Copy link

opbld33 commented Feb 15, 2022

Docs Build status updates of commit 7c54f3c:

✅ Validation status: passed

File Status Preview URL Details
xml/System.IO.Pipes/AnonymousPipeClientStream.xml ✅Succeeded View

For more details, please refer to the build report.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

For any questions, please:

@@ -34,7 +34,7 @@
</Base>
<Interfaces />
<Docs>
<summary>Exposes the client side of an anonymous pipe stream, which supports both synchronous and asynchronous read and write operations.</summary>
<summary>Exposes the client side of an anonymous pipe stream, which supports both synchronous and asynchronous (on non-Windows platforms) read and write operations.</summary>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is statement can be confusing to the end users, as AnonymousPipeClientStream derives from Stream and it offers ReadAsync and WriteAsync methods. These methods internally schedule sync work to thread pool and work fine, but without cancellation support.

I think that we should rather say that cancellation is not supported on Windows. @daveaglick what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think you're right - it's confusing as I initially wrote it. The fact that the underlying pipe is still synchronous does seem like an implementation detail that's not super important in these docs. I'll update with language that makes it clear its only cancellation that's not supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated this and ended up adding more details to (rather than remove) the remark further down. How does it look now?

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.

@daveaglick perfect! thank you! 👍

@opbld30
Copy link

opbld30 commented Feb 15, 2022

Docs Build status updates of commit f3becb1:

✅ Validation status: passed

File Status Preview URL Details
xml/System.IO.Pipes/AnonymousPipeClientStream.xml ✅Succeeded View

For more details, please refer to the build report.

Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report.

For any questions, please:

@adamsitnik adamsitnik merged commit 71c7efb into dotnet:main Feb 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants