[Patch] Allow listening on Unix domain sockets

Server NGINX
1

Hi.

The attached patch allows Nginx to listen on Unix domain sockets, like
this:

server {
listen unix:/tmp/nginx.sock;
server_name foobar.com;
root /webapps/foobar;
}

We use it to improve proxying performance; Unix domain sockets are
faster than TCP sockets.

I’d like to see this patch getting merged upstream. Could a maintainer
please review this patch?

With kind regards,
Hongli L.

2

+1 on this (assuming the patch is acceptable).

Regards,
Cliff

3

Is this not supported already? I don’t know for sure that it is, but
I was recently reading on the github blog about setting up nginx with
unicorn, and using domain sockets is how they do it:

Maybe they modified nginx themselves, but from the article it doesn’t
sound like it.

Anyway, if it’s not included in nginx already, then +1 from me too.

Nick

4

As Cliff said, definitely +1. Is it at all possible to roll this thing
as a module, or does it fundamentally have to be a patch?

  • Leo
5

Nick P. wrote:

Is this not supported already? I don’t know for sure that it is, but
I was recently reading on the github blog about setting up nginx with
unicorn, and using domain sockets is how they do it:

Unicorn! | The GitHub Blog

Maybe they modified nginx themselves, but from the article it doesn’t
sound like it.

Anyway, if it’s not included in nginx already, then +1 from me too.

No. This is about Nginx itself listening on a Unix socket, not proxying
to other servers that listen on Unix sockets. In the Github article only
Unicorn is listening on Unix sockets.

6

On Fri, Oct 23, 2009 at 03:12:33PM +0200, Hongli L. wrote:

faster than TCP sockets.

I’d like to see this patch getting merged upstream. Could a maintainer
please review this patch?

With kind regards,
Hongli L.

Attachments:
http://www.ruby-forum.com/attachment/4174/nginx-unix-sockets.diff

Thank you for the patch.
The attached version is more correct, in particular, it allows several
listen sockets:

server {
listen unix:/tmp/nginx1.sock;
server_name foobar1.com;
}

server {
listen unix:/tmp/nginx2.sock;
server_name foobar2.com;
}

7

Igor S. wrote:

Thank you for the patch.
The attached version is more correct, in particular, it allows several
listen sockets:

server {
listen unix:/tmp/nginx1.sock;
server_name foobar1.com;
}

server {
listen unix:/tmp/nginx2.sock;
server_name foobar2.com;
}

I confirm that the patch works. Thanks Igor!

8

Hello!

On Fri, Oct 23, 2009 at 03:12:33PM +0200, Hongli L. wrote:

We use it to improve proxying performance; Unix domain sockets are
faster than TCP sockets.

I’d like to see this patch getting merged upstream. Could a maintainer
please review this patch?

I’m not really maintainer, but here is the review.

[…]

diff --git a/src/core/ngx_inet.c b/src/core/ngx_inet.c
index 4c18036…59998be 100644
— a/src/core/ngx_inet.c
+++ b/src/core/ngx_inet.c
@@ -68,7 +68,9 @@ ngx_sock_ntop(struct sockaddr *sa, u_char *text, size_t len, ngx_uint_t port)
size_t n;
struct sockaddr_in6 *sin6;
#endif

  • struct sockaddr_un *sun;

This should be protected by #if (NGX_HAVE_UNIX_DOMAIN).

  • size_t path_len;

Using ‘plen’ here is just enough to be understood. And actually
it may be a good idea to use just one ‘n’ shared with INET6 case.

Whitespace damage here.

 switch (sa->sa_family) {

 case AF_INET:

@@ -108,6 +110,17 @@ ngx_sock_ntop(struct sockaddr *sa, u_char *text, size_t len, ngx_uint_t port)
return n;
#endif

  • case AF_UNIX:
  •    sun = (struct sockaddr_un *) sa;

This should be protected by #if (NGX_HAVE_UNIX_DOMAIN).

  •    path_len = strlen(sun->sun_path);

  •    if (path_len == 0) {

  •        return ngx_snprintf(text, len, "(unknown)") - text;

  •    } else {

  •        ngx_copy(text, (const u_char *) sun->sun_path, path_len);

Using const is wrong here, as it’s not in prototype at least in
some cases.

And it looks like you just trashed memory if buffer pointed by
text isn’t big enough, no?

Note well that buffers used in the code for ngx_sock_ntop() result
are sized against NGX_SOCKADDR_STRLEN, which is big enough to hold
ipv6 address, but may be smaller than needed for unix sockets.

@@ -2426,7 +2426,11 @@ ngx_http_set_keepalive(ngx_http_request_t *r)

 if (tcp_nodelay
     && clcf->tcp_nodelay

  •    && c->tcp_nodelay == NGX_TCP_NODELAY_UNSET)

  •    && c->tcp_nodelay == NGX_TCP_NODELAY_UNSET
  • #if (NGX_HAVE_UNIX_DOMAIN)
  •    && c->sockaddr->sa_family != AF_UNIX
  • #endif

Please keep preprocessor directive starting at position 0.

And note that ngx_tcp_nopush() / ngx_tcp_push() invocations
probably should be protected too.

Maxim D.

9

Igor S. wrote:

An updated patch version. It fixes socket name buffer size and deletes
socket files on clean exit.

Hi Igor. I’ve verified that your patch still works with Nginx 0.7.64. Do
you plan on including the patch soon?

Regards,
Hongli L.

10

On Sat, Oct 24, 2009 at 01:58:12AM +0200, Hongli L. wrote:

server {
listen unix:/tmp/nginx2.sock;
server_name foobar2.com;
}

I confirm that the patch works. Thanks Igor!

An updated patch version. It fixes socket name buffer size and deletes
socket files on clean exit.

11

Hongli L. wrote:

Hi Igor. I’ve verified that your patch still works with Nginx 0.7.64. Do
you plan on including the patch soon?

I mean inclusion in the 0.7 series. I see that it’s already been
included in the 0.8 series.

12

On Tue, Nov 17, 2009 at 11:57:19AM +0100, Hongli L. wrote:

Hongli L. wrote:

Hi Igor. I’ve verified that your patch still works with Nginx 0.7.64. Do
you plan on including the patch soon?

I mean inclusion in the 0.7 series. I see that it’s already been
included in the 0.8 series.

0.8 has many changes related to listening on unix sockets including

set_real_ip_from unix:;

There is still an issue with unix sockets: access rights.
Currently, master process creates the socket with root:wheel/0700
access right while it should use something like USER:GROUP/0777.

I plan to fix this in 0.8.28 and then I may make a complete patch to
test in on 0.7.