Skip to content

unify error handling for FileStream and Pipe Streams #77543

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

Merged
merged 7 commits into from
Nov 8, 2022

Conversation

adamsitnik
Copy link
Member

@adamsitnik adamsitnik commented Oct 27, 2022

This PR changes FileStream to match AnonymousPipe*Stream, NamedPipe*Stream edge case error handling. Namely:

  • when the server pipe disconnects or gets closed, a write to the client FileStream fails with an exception rather than reporting success and writing nothing (what we had so far).
  • when the server pipe disconnects, a read from FileStream returns zero rather than throwing an exception (what we had so far).

@adamsitnik adamsitnik added this to the 8.0.0 milestone Oct 27, 2022
@ghost ghost assigned adamsitnik Oct 27, 2022
@ghost
Copy link

ghost commented Oct 27, 2022

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

Issue Details

TODO:

  • Write methods
  • Test exclusions for platforms that don't support pipes like Linux Bionic etc
Author: adamsitnik
Assignees: -
Labels:

area-System.IO

Milestone: 8.0.0

@adamsitnik adamsitnik changed the title unify error handling for FileStream and PipeClient Streams unify error handling for FileStream and Pipe Streams Oct 28, 2022
@adamsitnik adamsitnik marked this pull request as ready for review October 28, 2022 10:10
public static class UnifiedErrorHandlingTests
{
[Fact]
public static void WhenAnonymousPipeServerIsClosedAnonymousPipeClientReadReturnsZero()
Copy link
Member

Choose a reason for hiding this comment

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

Nit:
I'm finding it very difficult to read and tell apart all of these different test names, including pulling out the salient details. Can you restructure them to a form something along the lines of:

AnonymousPipeServer_AnonymousPipeClient_ServerClosed_ReadReturnsZero
AnonymousPipeServer_FileStreamClient_ServerClosed_ReadReturnsZero
NamedPipeServer_FileStreamClient_ServerDisconnects_ReadReturnsZero
...

etc.?

Copy link
Member Author

Choose a reason for hiding this comment

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

@stephentoub can we meet somewhere in the middle?

- WhenAnonymousPipeServerIsClosedAnonymousPipeClientReadReturnsZero
+ When_AnonymousPipeServer_IsClosed_AnonymousPipeClient_ReadReturnsZero

Since we are using the [Fact] to annotate the tests I am trying to name in the way that they describe the fact they are testing.

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Thanks.

@adamsitnik
Copy link
Member Author

The test failures are unrelated (#78017), merging.

@adamsitnik adamsitnik merged commit 6bcc3e0 into dotnet:main Nov 8, 2022
@adamsitnik
Copy link
Member Author

@stephentoub thank you for the review! We are one step closer to PipeStream and File Stream code unification (preferably reuse)

@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants