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

Support setting NamedPipeClientStream.ReadMode with PipeAccessRights ctor overload #100001

Merged
merged 7 commits into from Mar 27, 2024

Conversation

jeffhandley
Copy link
Member

Fixes #83072 (Cannot set ReadMode after creating a NamedPipeClientStream instance).

Per the API Review notes and video:

  1. Add the missing constructor to NamedPipeClientStream that accepts PipeAccessRights
  2. Promote PipeAccessRights from System.IO.Pipes.AccessRights to System.IO.Pipes, and add the necessary type-forward
  3. Mark the constructor as being only supported on Windows, but leave the enum available on all platforms
  4. Review for nullability annotations -- no arguments allow null

The .NET Framework implementation was referenced from Pipe.cs, but the behavior was improved slightly.

  • Argument validation logic is reused rather than being duplicated into the new constructor
  • The direction argument is given a valid value when the desiredAccessRights value is invalid, ensuring that the correct desiredAccessRights argument exception is thrown
    • In netfx, any desiredAccessRights value missing both the ReadData (1) and WriteData (2) bits would throw an argument exception for direction, which is not an argument to the constructor
  • Argument validation for desiredAccessRights also throws for a 0 value, which netfx didn't do

The functionality was tested using the following server / client apps:

server.cs

using System.IO.Pipes;

var pipeOut = new NamedPipeServerStream("SomeNamedPipe",
                                        PipeDirection.Out,
                                        1,
                                        PipeTransmissionMode.Message);

Console.WriteLine("Waiting for a connection...");
pipeOut.WaitForConnection();

Console.WriteLine("Connected. Sending messages...");

while (true) {
    pipeOut.Write("Hello "u8);
    pipeOut.Write("from "u8);
    pipeOut.Write("the "u8);
    pipeOut.Write("server!"u8);

    Console.WriteLine("Messages sent. Sleeping for 10 seconds.");
    System.Threading.Thread.Sleep(10_000);
}

client.cs

using System.IO.Pipes;

var pipeIn = new NamedPipeClientStream(".",
                             "SomeNamedPipe",
                             PipeAccessRights.ReadData | PipeAccessRights.WriteAttributes,
                             PipeOptions.None,
                             System.Security.Principal.TokenImpersonationLevel.None,
                             System.IO.HandleInheritability.None);

pipeIn.Connect();
pipeIn.ReadMode = PipeTransmissionMode.Message;

while (pipeIn.IsConnected)
{
    var buffer = new byte[2];
    var bytesRead = pipeIn.Read(buffer, 0, buffer.Length);

    if (bytesRead > 0) {
        var message = System.Text.Encoding.UTF8.GetString(buffer, 0, bytesRead);
        Console.Write(message);

        System.Threading.Thread.Sleep(1000);

        if (pipeIn.IsMessageComplete) {
            Console.WriteLine();
        }
    }
}

With this PR, that application works as expected, with the client detecting completed messages after each word and rendering newlines.

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.

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.

Other than the comments on tests, LGTM. Thanks.

Copy link
Member

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

Looks good so far, thanks! Please also add some tests proving the new API works as expected.

Copy link
Member

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

Thanks! Left some more suggestions for you to consider.

Copy link
Member

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

LGTM!
And thanks for also moving those unrelated windows and unix tests to their own file, even thought they were not part of the PR.

@jeffhandley jeffhandley merged commit ddbb712 into dotnet:main Mar 27, 2024
87 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 27, 2024
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.

Cannot set ReadMode after creating a NamedPipeClientStream instance
3 participants