Fail hard when UNIX socket cannot be created #830
Conversation
src/pooler.c
Outdated
| @@ -239,7 +239,8 @@ static void create_unix_socket(const char *socket_dir, int listen_port) | |||
| unlink(un.sun_path); | |||
| } | |||
|
|
|||
| add_listen(AF_UNIX, (const struct sockaddr *)&un, addrlen); | |||
| if (!add_listen(AF_UNIX, (const struct sockaddr *)&un, addrlen)) | |||
| die("failed to create UNIX socket in directory: %s", socket_dir); | |||
There was a problem hiding this comment.
Agreed that we should make this an error somehow.
The error leading up to this is something like
WARNING cannot listen on unix:/tmpx/.s.PGSQL.6432: bind(): No such file or directory
so the directory was already mentioned. Also, the way you have phrased it, I would expect the error message to come after the colon, not the directory name. Maybe let's just simplify this to die("failed to create UNIX socket").
I don't think we use the capitalization "UNIX socket" anywhere.
Also, let's consider how this works in PostgreSQL: There, it checks whether at least one TCP/IP socket was created and at least one Unix socket, if configured. The current logic in PgBouncer is that it checks whether at least one socket was created in total. If we are going to touch this, should we make the logic more like PostgreSQL? That would include something like your patch and then another change to check the count of created TCP/IP sockets separately.
There was a problem hiding this comment.
I simplified the error message to "failed to create unix socket". Our capitalization of unix socket is inconsistent, using both unix and Unix. But I agree that it's best not to add a third version.
There was a problem hiding this comment.
Also, let's consider how this works in PostgreSQL: There, it checks whether at least one TCP/IP socket was created and at least one Unix socket, if configured. The current logic in PgBouncer is that it checks whether at least one socket was created in total. If we are going to touch this, should we make the logic more like PostgreSQL? That would include something like your patch and then another change to check the count of created TCP/IP sockets separately.
For this I created #838
There was a problem hiding this comment.
LGTM. Could you add a comment explaining this situation? Commit message contains an explanation but a comment is better (similar to the one in parse_add).
JelteF
force-pushed
the
fail-hard-unix-socket-failure
branch
2 times, most recently
from
e6b83e1 to
44c452e
Compare
When a user requests that a UNIX socket is created and this fails for some reason we ignored the error. This starts failing hard when that happens. This problem is especially bad when so_reuseport is used in combination with peering, because the peer list would then contain sockets that don't exist and forwarding cancels would wait for timeout and then fail silently.
JelteF
force-pushed
the
fail-hard-unix-socket-failure
branch
from
44c452e to
97ba16b
Compare
When a user requests that a UNIX socket is created and this fails for
some reason we ignored the error. With this change it starts failing
hard when that happens.
This problem is especially bad when so_reuseport is used in combination
with peering, because the peer list would then contain sockets that
don't exist and forwarding cancels would wait for timeout and then fail
silently.