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 Windows NamedPipe's in Kestrel #14207

Closed
6 tasks done
mconnew opened this issue Sep 20, 2019 · 18 comments
Closed
6 tasks done

Support Windows NamedPipe's in Kestrel #14207

mconnew opened this issue Sep 20, 2019 · 18 comments
Assignees
Labels
affected-very-few This issue impacts very few customers area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions 🥌 Bedrock enhancement This issue represents an ask for new feature or an enhancement to an existing one partner-impact severity-nice-to-have This label is used by an internal tool
Milestone

Comments

@mconnew
Copy link
Member

mconnew commented Sep 20, 2019

Is your feature request related to a problem? Please describe.

CoreWCF needs to be able to accept connections on Windows named pipes to accept connections from existing .NET Framework clients.

Describe the solution you'd like

CoreWCF needs to support creating a listening pipe on both local and global namespaces. We also need to be able to modify the ACL on the named pipe (PipeStream.GetAccessControl/SetAccessControl).


Update 2022/12/22 @JamesNK

Initial commit - #44426

Work still to do:

@blowdart
Copy link
Contributor

blowdart commented Oct 16, 2019

Tagging @danmoseley because this fits into the email discussion around permissions apis that will vary by platform

@mconnew
Copy link
Member Author

mconnew commented Oct 16, 2019

@blowdart, this will only be used by CoreWCF on Windows as we have a convoluted mechanism which needs to be followed to be compatible with existing WCF clients. But as WCF only uses this for localhost communication, it doesn't need to be supported on Linux. We can use the datagram sockets on Linux instead as there is no existing mechanism which we would need to be compatible with.

@ghost
Copy link

ghost commented Aug 19, 2020

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@jkotalik jkotalik added affected-very-few This issue impacts very few customers enhancement This issue represents an ask for new feature or an enhancement to an existing one severity-nice-to-have This label is used by an internal tool labels Nov 13, 2020 — with ASP.NET Core Issue Ranking
@MeikTranel
Copy link

Can we get this issue back in the roadmap?
It sounds like the underlying ability is already there and just needs to be exposed - grpc/grpc-dotnet#1112 (comment)

And i really disagree with the affected-very-few label, as this seems to be the one issue blocking the entire IPC story in .NET Core for those migrating from Framework. This would be incredibly helpful for CoreWCF as well as GRPC.

@danmoseley
Copy link
Member

@carlossanlop this sounds like it might have been addressed by your work to improve ACL API on pipes?

@mconnew
Copy link
Member Author

mconnew commented Mar 25, 2021

@danmoseley, @carlossanlop, is there also support for modifying the QOS parameters of the named pipe? By default, when a client connects to a named pipe, the server has impersonation permissions for the identity that the client had when connecting. I know the named pipe classes in .NET even allows you to provide a delegate to execute using that impersonation feature. WCF explicitly turns this off as the default behavior is a security issue. Without support for disabling this through managed api's, WCF can't keep it's goal of zero system native calls.

@JamesNK JamesNK removed their assignment Sep 26, 2022
@JamesNK JamesNK changed the title Support Windows NamedPipe's in bedrock Support Windows NamedPipe's in Kestrel Oct 7, 2022
@JamesNK JamesNK self-assigned this Oct 7, 2022
@JamesNK JamesNK modified the milestones: Backlog, .NET 8 Planning Oct 7, 2022
@ghost
Copy link

ghost commented Oct 7, 2022

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@simonferquel
Copy link
Contributor

Some feedback from Unity to help on this issue

Context

Since Unity 2022.2, we moved part of our compilation pipeline in a separate process (the IL Post processing phase). We use Grpc to talk to this process, using Unix sockets on Mac and Linux and... named pipes for Windows. The ILPP server is an ASP.Net GRPC service, and we developed a custom Listener to accept named pipes connections and pass them to the ASP.Net pipeline, in a very similar fashion as @JamesNK PR.

Failure cases

As mentioned in #46230, under high load and memory pressure, we have seen WaitForConnectionAsync throwing IOException with message "Broken pipe". We don't know if it is related, but on the client side we do some server readyness checks by using the file system API instead of relying on connection timeouts (in followup section, it will become clearer why). So we had to catch those exceptions and continue accepting new ones

Performance considerations

Overall, establishing a named pipe connection is super slow compared to Unix sockets, and we used a lot of time trying to mitigate that.

  • Named pipe connections are slow, especially under high CPU load:
    We found out in our CI, when lots of ILPP jobs are scheduled concurrently from individual processes, connections could take more than 10 seconds to complete. Even with a more aggressive server side listener. It lead us to introduce connectivity checks using File API (File.Exist on the namedpipe file) instead of using connection timeouts

  • Partial mitigation on the server side: multiple listeners
    One way that made our ILPP runner better scale on bursts of scheduled jobs, was to have multiple tasks accepting connections in parallel instead of a single one. The sweat spot we found, was to run one task per CPU core.

  • Reusing named pipe handles
    Once an ASP.Net connection is done with a NamedPipeServerStream (e.g. client disconnected), you can reuse the handle by calling "Disconnect" and then WaitConnectionAsync on it. As we have many short-lived connections in our case, it helped achieving better accept loop performance.

@JamesNK
Copy link
Member

JamesNK commented Jan 25, 2023

Great feedback.

I've done performance testing of request throughput over an established connection, but I didn't test the performance of establishing new connections. It sounds like something we should focus on.

Once an ASP.Net connection is done with a NamedPipeServerStream (e.g. client disconnected), you can reuse the handle by calling "Disconnect" and then WaitConnectionAsync on it. As we have many short-lived connections in our case, it helped achieving better accept loop performance.

I've considered doing that. I wasn't sure how much it saved. If we did something then we'd cache a limited number of server streams and then discard them. We don't want a situation where there is a burst of connections, and then the server is left holding hundreds/thousands of NamedPipeServerStream instances.

You mentioned you sometimes got broken pipes errors under very high load. I wonder if reusing NamedPipeServerStream is related?

One way that made our ILPP runner better scale on bursts of scheduled jobs, was to have multiple tasks accepting connections in parallel instead of a single one. The sweat spot we found, was to run one task per CPU core.

It's possible we could have multiple accept loops inside the listener and scale it based on cores.

@mconnew
Copy link
Member Author

mconnew commented Jan 25, 2023

I don't believe this isn't something inherent in named pipes, just in how they are being used. WCF will by default have 2 * Core count pending connects on the server side and we don't see these problems. We're using P/Invoke to the win32 api's and not using the managed wrappers though. I just checked our code and we would throw an exception where we would have seen it very easily if this was happening to us.

I would guess the broken pipe errors are caused by reusing the NamedPipeServerStream as that's the only consequential difference I could find between how WCF connects to named pipes and how NamedPipeServerStream does. Are you calling WaitForPipeDrain before calling Disconnect? How quickly does the client side close it's handle? WCF has a close handshake so both ends will close the handle at about the same time. We don't have the situation where there's a delay and the client then reads from the pipe discovering it was closed and closing the handle some time after the disconnect. The server only disconnects when the client is explicitly expecting it.

As for performance, named pipes used to be faster than sockets until Windows added a localhost fast path in the TCP stack (I think around Windows 8 timeframe). There's still not a lot in it though. @simonferquel, are you using PipeTransmissionMode of Byte or Message? There might be a performance difference. WCF uses Message mode, but it looks like NamedPipeServerStream uses Byte mode by default.

Client connect delay can happen with NamedPipeClientStream as the code has a race condition. It tries to connect to the pipe, and if there's no current pending listener (but the pipe does exist), it calls an api to wait for a listener. Then it tries to connect again. This is where there's the race condition, if you have two clients both trying to connect, the first one could return from the wait call, and the second one could connect to the available pipe before the first gets to it. The better thing to do is to use the connect call to wait for a listener, but there's no way to configure NamedPipeClientStream to do that, so we're left with the race condition which could get pathalogical for a particular client. There's no concept of FIFO on trying to connect to the next available server stream in the managed wrapper.

@simonferquel
Copy link
Contributor

simonferquel commented Jan 25, 2023

I would guess the broken pipe errors are caused by reusing the NamedPipeServerStream as that's the only consequential difference I could find between how WCF connects to named pipes and how NamedPipeServerStream does.

We have seen this before we implemented stream reuses. It might be related to the race condition in NamedPipeClientStream you mentioned, as IIRC, the broken pipe appears on the server side, but I don't think we have connection failing on the client side. It might be hidden by the Grpc RetryPolicy we are using though... as we did put one in place because we also see (very rarely) Pipe broken errors on the client after the connection is established when the HttpClient actually send bytes to the server.

Are you calling WaitForPipeDrain before calling Disconnect? How quickly does the client side close it's handle?

We do if the stream is still connected (e.g. the disconnection is driven by the server) but it happens very rarely, as in our case, most connections are disposed from the client side first. Also to behave as close as possible as our real time scenarios (where the Grpc call is made trough a very short leaved NativeAOT compiled executable run by our job scheduler), our stress test disposes the HttpHandler on each Grpc call. We also tried to implement a "clean" shutdown, but exception on WaitForConnectionAsync still ocured - and everything was slowed down.

are you using PipeTransmissionMode of Byte or Message? There might be a performance difference. WCF uses Message mode, but it looks like NamedPipeServerStream uses Byte mode by default.

We tried both, impact on performance is basically undistinguishable from noise.

@JamesNK
Copy link
Member

JamesNK commented Jan 25, 2023

I wrote a stress test as part of #46259 and found I could get named pipes to throw IOException.

@JamesNK
Copy link
Member

JamesNK commented Feb 7, 2023

@simonferquel I've made improvements to the named pipe transport based on your feedback and added some benchmarks:

Connection establishment and one HTTP request round trip:

  • Before 59.16 us
  • After 23.74 us (16 accept tasks + reuse NamedPipeServerStream)

A 60% improvement is pretty good.

What are you using as the client in the Unity build IPC scenario? Is it .NET as well? I wrote docs around named pipes with gRPC and IPC. I'm curious whether you're using SocketsHttpHandler.ConnectCallback.

@danmoseley
Copy link
Member

@JamesNK could you clarify what is left here?

@simonferquel
Copy link
Contributor

Totally missed your last comment, sorry! Yes we use a .net client (and we build it with nativeaot on supporte platforms)

@mconnew
Copy link
Member Author

mconnew commented Mar 28, 2023

@JamesNK, I believe in the docs it's worth recommending adding the ACL to deny clients connecting from remote machines. As things stand, the example code would allow cross machine communication, unless you hard coded in that limitation to the implementation?

@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 2, 2023
@adityamandaleeka
Copy link
Member

@JamesNK is this done now?

@JamesNK
Copy link
Member

JamesNK commented Oct 3, 2023

DONE

@JamesNK JamesNK closed this as completed Oct 3, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affected-very-few This issue impacts very few customers area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions 🥌 Bedrock enhancement This issue represents an ask for new feature or an enhancement to an existing one partner-impact severity-nice-to-have This label is used by an internal tool
Projects
None yet
Development

No branches or pull requests