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.

Sign up for GitHub

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

Jump to bottom

Add PipeOptions.FirstPipeInstance enum value #83936

Merged
merged 11 commits into from May 30, 2023
Merged

Add PipeOptions.FirstPipeInstance enum value #83936

merged 11 commits into from May 30, 2023

Conversation

Tarun047
Copy link
Contributor

Added a new Enum Value to represent expose the FILE_FLAG_FIRST_PIPE_INSTANCE flag specific to Windows Kernel API to create named pipes.
Resolve #76984

@dotnet-issue-labeler
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.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Mar 26, 2023
@ghost
Copy link

ghost commented Mar 26, 2023

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

Issue Details

Added a new Enum Value to represent expose the FILE_FLAG_FIRST_PIPE_INSTANCE flag specific to Windows Kernel API to create named pipes.
Resolve #76984

Author: Tarun047
Assignees: -
Labels:

area-System.IO, new-api-needs-documentation
Milestone: -

@build-analysis build-analysis bot mentioned this pull request Mar 26, 2023
Closed
@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Apr 3, 2023
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Apr 9, 2023
@Tarun047 Tarun047 requested a review from Jozkee April 10, 2023 05:46
adamsitnik
adamsitnik reviewed May 19, 2023
View reviewed changes
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.

@Tarun047 big thanks for your contribution!

Could you please address @stephentoub feedback and let me know whether you could experiment with implementing it also for Unix?

src/libraries/System.IO.Pipes/src/Resources/Strings.resx Outdated Show resolved Hide resolved
src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeOptions.cs Outdated Show resolved Hide resolved
@adamsitnik adamsitnik added the needs-author-action An issue or pull request that requires more info or actions from the author. label May 19, 2023
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label May 20, 2023
This was referenced May 20, 2023
Open
Closed
adamsitnik
adamsitnik reviewed May 22, 2023
View reviewed changes
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.

However within the same process we use the static dictionary to check if a server for the given path already exists.
I've changed the code in this area such that it checks if there is firstPipeInstance applied and throw exception.

Great catch!

I've add some comments, we need only a little bit of polishing before merging the PR.

@Tarun047 big thanks for working on it!

src/libraries/System.IO.Pipes/src/System/IO/Pipes/NamedPipeServerStream.cs Outdated Show resolved Hide resolved
src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CreateServer.cs Show resolved Hide resolved
src/libraries/System.IO.Pipes/src/System/IO/Pipes/NamedPipeServerStream.Unix.cs Show resolved Hide resolved
src/libraries/System.IO.Pipes/src/System/IO/Pipes/NamedPipeServerStream.Unix.cs Outdated Show resolved Hide resolved
adamsitnik
adamsitnik reviewed May 22, 2023
View reviewed changes
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.

However within the same process we use the static dictionary to check if a server for the given path already exists.
I've changed the code in this area such that it checks if there is firstPipeInstance applied and throw exception.

Great catch!

I've add some comments, we need only a little bit of polishing before merging the PR.

@Tarun047 big thanks for working on it!

@adamsitnik adamsitnik dismissed Jozkee’s stale review May 22, 2023 11:14

The concerns have been addressed.

@adamsitnik adamsitnik self-assigned this May 23, 2023
@Tarun047 Tarun047 requested a review from adamsitnik May 29, 2023 14:30
@build-analysis build-analysis bot mentioned this pull request May 29, 2023
Closed
adamsitnik
adamsitnik approved these changes May 29, 2023
View reviewed changes
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.

Excellent, great job @Tarun047 !

The failures are unrelated (#86861, dotnet/arcade#11683). I'll re-run the tests that were cancelled due to timeout.

@adamsitnik
Copy link
Member

adamsitnik commented May 30, 2023 •
edited

I'll re-run the tests that were cancelled due to timeout.

No luck, will try to repro the timeout locally.

Edit: the timeouts happen in other PRs as well and are being tracked by #76454

@adamsitnik adamsitnik merged commit d4b31a8 into dotnet:main May 30, 2023
99 of 105 checks passed
@adamsitnik adamsitnik added this to the 8.0.0 milestone May 30, 2023
@dotnet dotnet locked as resolved and limited conversation to collaborators Jun 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@adamsitnik adamsitnik adamsitnik approved these changes

@Jozkee Jozkee Awaiting requested review from Jozkee

@stephentoub stephentoub Awaiting requested review from stephentoub

Assignees

@adamsitnik adamsitnik

Labels
area-System.IO community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.

[API Proposal]: Add PipeOptions.FirstPipeInstance enum value
4 participants
@Tarun047 @adamsitnik @stephentoub @Jozkee