-
Notifications
You must be signed in to change notification settings - Fork 522
Conversation
Hi @Daniel15, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! TTYL, DNFBOT; |
@Daniel15, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
Thanks for the PR. Have you tested Nginx proxying to a UNIX socket with Testing this should ensure that our logic to load balance across multiple libuv event loops works with UNIX sockets. |
{ | ||
private static readonly Action<UvStreamHandle, int, Exception, object> _connectionCallback = ConnectionCallback; | ||
protected T ListenSocket { get; private set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason this couldn't just be a UvStreamHandle instead of being generic?
I don't think you need to use generics for any of your changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was just in case I needed to call a method specific to the particular implementation. I don't think I ended up needing to do that though, so it could probably just be UvStreamHandle
.
I believe I did test this, I'll test it out again tonight and confirm though. Unfortunately I forgot to include my test plan in my commit message. |
Have you done a performance comparison between running an Nginx reverse-proxy over the Unix socket vs a TCP socket? If not I'm happy to do it myself and share the results; I just don't want to spend time if you've got numbers already. |
@markrendle I haven't done any perf testing. |
@halter73 - I just tested it again on Debian Linux + Mono 4.0.2 and it worked fine. These were the changes I made when testing: diff --git a/samples/SampleApp/Startup.cs b/samples/SampleApp/Startup.cs
index 5f81cbe..fddd1c9 100644
--- a/samples/SampleApp/Startup.cs
+++ b/samples/SampleApp/Startup.cs
@@ -2,6 +2,8 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
using System;
+using Kestrel;
using Microsoft.AspNet.Builder;
using Microsoft.AspNet.Http;
@@ -11,6 +13,10 @@ public class Startup
{
public void Configure(IApplicationBuilder app)
{
+ var server = (IKestrelServerInformation) app.Server;
+ server.ThreadCount = 10;
+ Console.WriteLine("Threads: {0}", server.ThreadCount);
+
app.Run(async context =>
{
Console.WriteLine("{0} {1}{2}{3}",
I also tested on Mac OS, where it seems broken even without my changes (I get "Unknown system error"). I'll submit a separate issue for that. |
I just rebased and rebuilt and now it works on Mac OS X for me. Strange. |
@markrendle Are you still planning to do the performance comparison between this and proxying via a TCP socket? It would be nice to know since the main benefit of using a UNIX socket seems to be efficiency. Thanks! |
The changes look weird now. I see a bunch of unrelated commits (likely from the merge). Can you clean up the PR and make sure the diff only shows your changes? |
@davidfowl - Oops, looks like something weird happened when I was merging master into my branch. This commit looks like the right one: Daniel15@0bb837c. I'm not very experienced with Git merging so I'll see if I can figure out how to clean it up tomorrow. |
The common use-case for Kestrel in production will be behind a reverse proxy such as Nginx. In cases where the reverse proxy is located on the same machine as the application, connecting via a UNIX socket is more efficient than a TCP socket, as it avoids going through the network layer. Accessing 127.0.0.1 through TCP still needs to initiate a TCP connection and perform handshaking, checksumming, etc, all of which is avoided by using UNIX sockets. - Moved TCP-specific stuff from Listener into new TcpListener class (same with ListenerPrimary and ListenerSecondary) - Made Listener abstract - Created new PipeListener. Note that while the use case is for UNIX sockets, this is called "Pipe" in uv, so I've called this "PipeListener" so the terminology is consistant - Uses "unix" URL scheme to determine whether to use socket. "http://127.0.0.1:5000" is for listening via TCP while "unix:///var/run/kestrel-test.sock" is for listening via UNIX socket aspnet#156
@davidfowl - It should be fixed now, let me know if it still looks weird for you :) |
@halter73 I am still planning to do the perf testing. I'm literally just finishing up on porting a monolithic ASP.NET MVC 5 application to Docker-hosted DNX MVC 6 microservices, then I'll have some time to set up a test environment with Nginx proxying to Kestrel over both TCP and a Socket. I'll post the results when I have them. |
@@ -43,7 +43,8 @@ public IDisposable Start(IServerInformation serverInformation, Func<IFeatureColl | |||
{ | |||
disposables.Add(engine.CreateServer( | |||
address.Scheme, | |||
address.Host, | |||
// Unix sockets use a file path, not a hostname. | |||
address.Scheme == Constants.UnixScheme ? address.Path : address.Host, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you specify the address since the host is ignored. Do you use triple forward slash? e.g:
unix:///tmp/kestrel-test.sock/
Edit: Nvm. I noticed this is exactly what you do in the sample project.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, triple slash. I based that on the file
URL scheme which uses an empty host section to represent localhost, and on other apps that use unix:///
as the prefix for UNIX socket paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we include the scheme (e.g. http, https, fastcgi) in the address? Sorta like Nginix does in its proxy_pass directive, i.e. http://unix:/tmp/kestrel-test.sock.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, looking at how unix:
is used it appears it's treated as the "host" information rather than the "scheme". Which kind of makes sense - it determines where to connect/listen rather than how you frame data on the stream after the connection.
Also, if fastcgi is added as a way to frame data - that would likely appear to kestrel as a different scheme which would work on either tcp network sockets or unix domain sockets.
So all of the below would be valid:
http://127.0.0.1:8080/
http://unix:/tmp/my-server/:/
fastcgi://127.0.0.1:9090/
fastcgi://unix:/tmp/my-other-server/:/
So that would involve two changes... Kestrel needs to understand unix:*:
is a valid host format, allowing everything after the second : to become the path, and then you would look for unix:
leading characters in the host to tell that it's not a tcp connection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is :
a valid character in the hostname section of a URI? Would the parser allow http://unix:/tmp/foo
as a valid URI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, but we don't use the System.Uri parser for any of this. We have to support wildcards anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lodejard Here is a change to ServerAddress that supports your suggested URI syntax.
Does this look like what you want? If so, I can start adding badly needed URI parsing tests for this.
Thanks! 😄 |
The common use-case for Kestrel in production will be behind a reverse proxy such as Nginx. In cases where the reverse proxy is located on the same machine as the application, connecting via a UNIX socket is more efficient than a TCP socket, as it avoids going through the network layer. Accessing 127.0.0.1 through TCP still needs to initiate a TCP connection and perform handshaking, checksumming, etc, all of which is avoided by using UNIX sockets.
Example Nginx configuration for proxying to a UNIX socket:
#156