Unverified Commit fac188b4 authored by Binbin's avatar Binbin Committed by GitHub
Browse files

Introduce socket shutdown into connection type, used if a fork is active (#11376)

Introduce socket `shutdown()` into connection type, and use it
on normal socket if a fork is active. This allows us to close
client connections when there are child processes sharing the
file descriptors.

Fixes #10077. The reason is that since the `fork()` child is holding
the file descriptors, the `close` in `unlinkClient -> connClose`
isn't sufficient. The client will not realize that the connection is
disconnected until the child process ends.

Let's try to be conservative and only use shutdown when the fork is active.
parent c337c0a8
......@@ -80,9 +80,10 @@ typedef struct ConnectionType {
int (*addr)(connection *conn, char *ip, size_t ip_len, int *port, int remote);
int (*listen)(connListener *listener);
/* create/close connection */
/* create/shutdown/close connection */
connection* (*conn_create)(void);
connection* (*conn_create_accepted)(int fd, void *priv);
void (*shutdown)(struct connection *conn);
void (*close)(struct connection *conn);
/* connect & accept */
......@@ -240,6 +241,10 @@ static inline int connSetWriteHandlerWithBarrier(connection *conn, ConnectionCal
return conn->type->set_write_handler(conn, func, barrier);
}
static inline void connShutdown(connection *conn) {
conn->type->shutdown(conn);
}
static inline void connClose(connection *conn) {
conn->type->close(conn);
}
......
......@@ -1448,6 +1448,8 @@ void unlinkClient(client *c) {
}
}
}
/* Only use shutdown when the fork is active and we are the parent. */
if (server.child_type) connShutdown(c->conn);
connClose(c->conn);
c->conn = NULL;
}
......
......@@ -125,6 +125,12 @@ static int connSocketConnect(connection *conn, const char *addr, int port, const
* move here as we implement additional connection types.
*/
static void connSocketShutdown(connection *conn) {
if (conn->fd == -1) return;
shutdown(conn->fd, SHUT_RDWR);
}
/* Close the connection and free resources. */
static void connSocketClose(connection *conn) {
if (conn->fd != -1) {
......@@ -388,9 +394,10 @@ static ConnectionType CT_Socket = {
.addr = connSocketAddr,
.listen = connSocketListen,
/* create/close connection */
/* create/shutdown/close connection */
.conn_create = connCreateSocket,
.conn_create_accepted = connCreateAcceptedSocket,
.shutdown = connSocketShutdown,
.close = connSocketClose,
/* connect & accept */
......
......@@ -748,6 +748,19 @@ static int connTLSListen(connListener *listener) {
return listenToPort(listener);
}
static void connTLSShutdown(connection *conn_) {
tls_connection *conn = (tls_connection *) conn_;
if (conn->ssl) {
if (conn->c.state == CONN_STATE_CONNECTED)
SSL_shutdown(conn->ssl);
SSL_free(conn->ssl);
conn->ssl = NULL;
}
connectionTypeTcp()->shutdown(conn_);
}
static void connTLSClose(connection *conn_) {
tls_connection *conn = (tls_connection *) conn_;
......@@ -1103,9 +1116,10 @@ static ConnectionType CT_TLS = {
.addr = connTLSAddr,
.listen = connTLSListen,
/* create/close connection */
/* create/shutdown/close connection */
.conn_create = connCreateTLS,
.conn_create_accepted = connCreateAcceptedTLS,
.shutdown = connTLSShutdown,
.close = connTLSClose,
/* connect & accept */
......
......@@ -103,6 +103,10 @@ static void connUnixAcceptHandler(aeEventLoop *el, int fd, void *privdata, int m
}
}
static void connUnixShutdown(connection *conn) {
connectionTypeTcp()->shutdown(conn);
}
static void connUnixClose(connection *conn) {
connectionTypeTcp()->close(conn);
}
......@@ -162,9 +166,10 @@ static ConnectionType CT_Unix = {
.addr = connUnixAddr,
.listen = connUnixListen,
/* create/close connection */
/* create/shutdown/close connection */
.conn_create = connCreateUnix,
.conn_create_accepted = connCreateAcceptedUnix,
.shutdown = connUnixShutdown,
.close = connUnixClose,
/* connect & accept */
......
......@@ -54,6 +54,44 @@ start_server {tags {"introspection"}} {
# After killing `me`, the first ping will throw an error
assert_error "*I/O error*" {r ping}
assert_equal "PONG" [r ping]
$rd1 close
$rd2 close
$rd3 close
$rd4 close
}
test "CLIENT KILL close the client connection during bgsave" {
# Start a slow bgsave, trigger an active fork.
r flushall
r set k v
r config set rdb-key-save-delay 10000000
r bgsave
wait_for_condition 1000 10 {
[s rdb_bgsave_in_progress] eq 1
} else {
fail "bgsave did not start in time"
}
# Kill (close) the connection
r client kill skipme no
# In the past, client connections needed to wait for bgsave
# to end before actually closing, now they are closed immediately.
assert_error "*I/O error*" {r ping} ;# get the error very quickly
assert_equal "PONG" [r ping]
# Make sure the bgsave is still in progress
assert_equal [s rdb_bgsave_in_progress] 1
# Stop the child before we proceed to the next test
r config set rdb-key-save-delay 0
r flushall
wait_for_condition 1000 10 {
[s rdb_bgsave_in_progress] eq 0
} else {
fail "bgsave did not stop in time"
}
}
test {MONITOR can log executed commands} {
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment