Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Implement support for UNIX sockets #157

Merged
merged 1 commit into from
Aug 24, 2015
Merged

Conversation

Daniel15
Copy link
Contributor

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

Example Nginx configuration for proxying to a UNIX socket:

server {
  server_name kestreltest.localhost;
  root /var/www/dan.cx/net5test/wwwroot;

  location / {
    # Serve static files directly; proxy everything else to Kestrel
    try_files $uri @aspnet;
  }

  location @aspnet {
    proxy_pass http://unix:/tmp/kestrel-test.sock;
    proxy_set_header Host $host;
  }
}

#156

@dnfclas
Copy link

dnfclas commented Aug 10, 2015

Hi @Daniel15, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

@dnfclas
Copy link

dnfclas commented Aug 10, 2015

@Daniel15, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

@halter73
Copy link
Member

Thanks for the PR. Have you tested Nginx proxying to a UNIX socket with IKestrelServerInformation.ThreadCount > 1? It looks like you probably made the necessary changes for this to work, but I want to make sure you tried it out. You can get at the IKestrelServerInformation object in your app startup code by casting IApplicationBuilder.Server.

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; }
Copy link
Member

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.

Copy link
Contributor Author

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.

@Daniel15
Copy link
Contributor Author

Have you tested Nginx proxying to a UNIX socket with IKestrelServerInformation.ThreadCount > 1

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.

@ThatRendle
Copy link
Contributor

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.

@Daniel15
Copy link
Contributor Author

@markrendle I haven't done any perf testing.

@Daniel15
Copy link
Contributor Author

@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.

@Daniel15
Copy link
Contributor Author

I just rebased and rebuilt and now it works on Mac OS X for me. Strange.

@halter73
Copy link
Member

@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!

@davidfowl
Copy link
Member

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?

@Daniel15
Copy link
Contributor Author

@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
@Daniel15
Copy link
Contributor Author

@davidfowl - It should be fixed now, let me know if it still looks weird for you :)

@ThatRendle
Copy link
Contributor

@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,
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Member

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.

@Daniel15
Copy link
Contributor Author

Thanks! 😄

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants