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

NamedPipeClientStream Connect with SafePipeHandle #110075

Closed
wants to merge 3 commits into from

Conversation

koenigst
Copy link
Contributor

@koenigst koenigst commented Nov 22, 2024

  • Checked that isConnected in the NamedPipeClientStream constructor is always true.
  • Created a new constructor without the argument.
  • Marked the old constructor as obsolete.
  • Corrected the nullability in the interop declarations.

I am aware that I did slightly more than in the original issue. Therefore I created a base commit and two additional commits that can be easily removed from this PR if desired.

Fixes #32760

* Check that isConnected in the NamedPipeClientStream constructor is always true.
* Added an argument test.

Fixes dotnet#32760
* Created a new constructor without the argument.
* Marked the old constructor as obsolete.

Fixes dotnet#32760
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

1 similar comment
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Nov 22, 2024
Comment on lines +92 to +103
[Obsolete]
public NamedPipeClientStream(PipeDirection direction, bool isAsync, bool isConnected, SafePipeHandle safePipeHandle)
: this(direction, isAsync, safePipeHandle)
{
if (!isConnected)
{
throw new ArgumentOutOfRangeException(nameof(isConnected));
}
}

// Create a NamedPipeClientStream from an existing server pipe handle.
public NamedPipeClientStream(PipeDirection direction, bool isAsync, SafePipeHandle safePipeHandle)
Copy link
Member

Choose a reason for hiding this comment

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

This would need API review, which isn't present in linked issue.

@stephentoub
Copy link
Member

Thanks for trying to help out! New surface area needs to first be reviewed and approved, following:
https://github.com/dotnet/runtime/blob/main/docs/project/api-review-process.md
Either a new issue should be opened proposing the new surface area, or the existing issue should be updated into a proposal. Either way, though, we can't accept this PR until such time that a new member has been agreed upon, so I'm going to close this. If/when an API is approved, you're of course welcome to re-open. Thanks!

@koenigst
Copy link
Contributor Author

@stephentoub I'm sorry for the inconvenience. I created a new PR (#110175) without the changes to the public API.

@stephentoub
Copy link
Member

No worries. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.IO community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NamedPipeClientStream Connect can use uninitialized _normalizedPipePath
3 participants