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 Kestrel named pipes transport #44426

Merged
merged 2 commits into from
Dec 22, 2022
Merged

Add Kestrel named pipes transport #44426

merged 2 commits into from
Dec 22, 2022

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Oct 8, 2022

Addresses #14207

@davidfowl
Copy link
Member

I do wonder if it makes sense to also build a Stream based transport mechanism that does all of this boilerplate that would then support any sort of duplex stream implementation.

@JamesNK
Copy link
Member Author

JamesNK commented Oct 9, 2022

I think there are always going to be small differences in each implementation. For example, QuicStream is a Stream, but we need to use WriteAsync(Memory<byte> buffer, bool complete) and QuicStream.ReadsComplete in the inner loops. And we need to react differently depending upon QUIC exceptions.

But in saying that, named pipe streams are pretty vanilla usages of Stream. A little too vanilla actually. I'm not sure if it is hidden by the named pipe stream abstraction or not, but there doesn't appear to be any way to indicate whether a named pipe ended gracefully or was aborted/connection lost. The only way to end a named pipe stream from the server is calling Disconnect (or dispose), and ReadAsync just returns 0 when the other side of the pipe disconnects.

@JamesNK
Copy link
Member Author

JamesNK commented Oct 9, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@davidfowl
Copy link
Member

I think there are always going to be small differences in each implementation. For example, QuicStream is a Stream, but we need to use WriteAsync(Memory buffer, bool complete) and QuicStream.ReadsComplete in the inner loops. And we need to react differently depending upon QUIC exceptions.

But in saying that, named pipe streams are pretty vanilla usages of Stream. A little too vanilla actually. I'm not sure if it is hidden by the named pipe stream abstraction or not, but there doesn't appear to be any way to indicate whether a named pipe ended gracefully or was aborted/connection lost. The only way to end a named pipe stream from the server is calling Disconnect (or dispose), and ReadAsync just returns 0 when the other side of the pipe disconnects.

Maybe? But we should have it as an option. The HttpClient support is similar, and you need to implement a wrapper stream to change that behavior.

@mconnew
Copy link
Member

mconnew commented Oct 13, 2022

With regards detecting a named pipe closing, it's probably even more complicated than you think. Although I haven't got to running the code yet (battling with project issues), you can see my attempt to duplicate the native P/Invoke based logic from .NET Framework using PipeStream on the close path here. You can see that we explicitly send a 0 byte write on the connection. I just checked the implementation of PipeStream.WriteAsync and it looks like we might have a problem here as it converts a zero byte write into a no-op and doesn't do anything. Based on the .NET Framework WCF named pipes implementation, a zero byte write should be actually sent to the underlying OS which pops out as a zero byte read on the receiving end signaling shutting down of the connection. I can probably fix this by getting the native handle and doing a P/Invoke to send a 0 byte write if needed. But I suspect there's a behavior change needed in PipeStream to get the close path working correctly. It's probably going to need some experimentation to know for sure.

@JamesNK JamesNK force-pushed the jamesnk/namedpipes-transport branch from cfcc8ea to c1a8126 Compare October 30, 2022 04:50
@JamesNK JamesNK changed the base branch from main to jamesnk/kestrel-multiple-transports October 30, 2022 04:52
Base automatically changed from jamesnk/kestrel-multiple-transports to main November 1, 2022 20:35
@JamesNK JamesNK force-pushed the jamesnk/namedpipes-transport branch from 5c2c1d6 to ee10898 Compare November 2, 2022 01:17
@@ -42,6 +42,8 @@ public static IWebHostBuilder UseKestrel(this IWebHostBuilder hostBuilder)
// Don't override an already-configured transport
services.TryAddSingleton<IConnectionListenerFactory, SocketTransportFactory>();

services.AddNamedPipes();
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to add the named pipes transport by default? Could this hurt trimming? @eerhardt

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't expect name pipes would include much. The types are thin wrappers around Win32 APIs on Windows and System.Net.Socket on Linux. However, it would be good to measure.

If the named pipes transport factory isn't registered by default, then we'd need to make sure that calling ListenNamePipe has a clear error message about how to register it.

Copy link
Member

Choose a reason for hiding this comment

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

It will hurt trimming but I'm sure we're going to end up with a new API that turns most of these off by default.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not a problem if excluded by empty/slim builder.

@JamesNK JamesNK changed the title Experiment with named pipes transport Add Kestrel named pipes transport Nov 12, 2022
/// <param name="pipeName">The name of the pipe.</param>
/// <param name="serverName">The name of the remote computer to connect to, or "." to specify the local computer.</param>
public NamedPipeEndPoint(string pipeName, string serverName)
{
Copy link
Member

Choose a reason for hiding this comment

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

This dovetails into the conversation going on in the thread model. Do you want to support connecting to other hosts? Is allowing a serverName intended to make this api represent the concept of a NamedPipe endpoint and as it has a server name, you're enabling representing it? Or is the intention to allow cross machine communication?

Copy link
Member

Choose a reason for hiding this comment

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

I just followed the flow of this object through and I'm convinced that for a service side application, including serverName doesn't make any sense. You only pass the path to NamedPipeServerStream as you can't create a named pipe on another host. NamedPipeServerStream inserts \\.\pipe\ in front of the pipe name that you pass.

Copy link
Member Author

Choose a reason for hiding this comment

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

We added the server name here if someone wants to use this endpoint in a client. When Kestrel starts up, it validates that the server name is ..

{
// Based on format at https://learn.microsoft.com/windows/win32/ipc/pipe-names
return $@"\\{ServerName}\pipe\{PipeName}";
}
Copy link
Member

Choose a reason for hiding this comment

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

It's a little bit more complicated than this. If the app is running in a Win32 App Container (think Windows Store app, NOT docker style container), then the pipe name needs to be of the format "\\{ServerName}\pipe\Local\{PipeName}" as apps in a Win32 app container can't create named pipes in a non-local namespace. Or do you expect users to include the Local\ part passed in as the pipeName?

Copy link
Member Author

Choose a reason for hiding this comment

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

The user would add Local\ to the pipe name. PipeName is passed directly to the pipe ctor, and it expects local as part of the name, e.g. new NamedPipeServerStream(@"Local\MyPipeName")

@adityamandaleeka adityamandaleeka added the blog-candidate Consider mentioning this in the release blog post label Dec 7, 2022
@ghost
Copy link

ghost commented Dec 7, 2022

@JamesNK, this change will be considered for inclusion in the blog post for the release it'll ship in. Nice work!

Please ensure that the original comment in this thread contains a clear explanation of what the change does, why it's important (what problem does it solve?), and, if relevant, include things like code samples and/or performance numbers.

This content may not be exactly what goes into the blog post, but it will help the team putting together the announcement.

Thanks!

@JamesNK JamesNK merged commit dfd4cde into main Dec 22, 2022
@JamesNK JamesNK deleted the jamesnk/namedpipes-transport branch December 22, 2022 03:02
@ghost ghost added this to the 8.0-preview1 milestone Dec 22, 2022
private readonly MemoryPool<byte> _memoryPool;
private readonly PipeOptions _inputOptions;
private readonly PipeOptions _outputOptions;
private readonly Mutex _mutex;
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems this is never used

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah nvm, it is used for cross-process protection, I see it from the connection factory

@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions blog-candidate Consider mentioning this in the release blog post
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants