[PATCH v2 00/14] Simple IPC Mechanism
- From
- Jeff Hostetler via GitGitGadget <gitgitgadget@gmail.com>
- Date
- Feb 1, 2021, 19:45 UTC
- Message-ID
- <pull.766.v2.git.1612208747.gitgitgadget@gmail.com>
- In-Reply-To
- <pull.766.git.1610465492.gitgitgadget@gmail.com>
Here is version 2 of my "Simple IPC" series and addresses the following review comments:
[1] Redo packet_write_gently() to take a scratch buffer argument and fixup callers to avoid potential thread-stack problems caused by a very large stack buffer when used multi-threaded callers. This turned out to be a little more involved than anticipated because the pkt-line code doesn't know about its thread state nor an opportunity to initialize thread-state.
[2] Deleted the unix_stream_socket() helper function and inline it in the few callers and then let those call sites decide whether to call die() or not.
[3] Refactor unix_stream_listen() to take an "options" structure and to incorporate the changes I described in my earlier unix_stream_listen_gently().
[4] Update unix_stream_connect() to return errors rather than calling die().
[5] Update the simple-ipc server startup to detect dead and/or in-use Unix domain sockets and/or create a new socket in a race-friendly way. I now use a variation of the atomic lock-rename-trick when creating the socket (details are in a large comment in the code).
Jeff Hostetler (10): pkt-line: promote static buffer in packet_write_gently() to callers pkt-line: add write_packetized_from_buf2() that takes scratch buffer simple-ipc: design documentation for new IPC mechanism simple-ipc: add win32 implementation simple-ipc: add t/helper/test-simple-ipc and t0052 unix-socket: elimiate static unix_stream_socket() helper function unix-socket: add options to unix_stream_listen() unix-socket: add no-chdir option to unix_stream_listen() unix-socket: do not call die in unix_stream_connect() simple-ipc: add Unix domain socket implementation
Johannes Schindelin (3):
pkt-line: optionally skip the flush packet in
write_packetized_from_buf()
pkt-line: (optionally) libify the packet readers
pkt-line: accept additional options in read_packetized_to_strbuf()Junio C Hamano (1): ci/install-depends: attempt to fix "brew cask" stuff
Documentation/technical/api-simple-ipc.txt | 34 + Makefile | 8 + builtin/credential-cache--daemon.c | 3 +- ci/install-dependencies.sh | 8 +- compat/simple-ipc/ipc-shared.c | 28 + compat/simple-ipc/ipc-unix-socket.c | 1127 ++++++++++++++++++++ compat/simple-ipc/ipc-win32.c | 751 +++++++++++++ config.mak.uname | 2 + contrib/buildsystems/CMakeLists.txt | 6 + convert.c | 4 +- pkt-line.c | 70 +- pkt-line.h | 26 +- simple-ipc.h | 230 ++++ t/helper/test-simple-ipc.c | 485 +++++++++ t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + t/t0052-simple-ipc.sh | 129 +++ unix-socket.c | 67 +- unix-socket.h | 16 +- 19 files changed, 2949 insertions(+), 47 deletions(-) create mode 100644 Documentation/technical/api-simple-ipc.txt create mode 100644 compat/simple-ipc/ipc-shared.c create mode 100644 compat/simple-ipc/ipc-unix-socket.c create mode 100644 compat/simple-ipc/ipc-win32.c create mode 100644 simple-ipc.h create mode 100644 t/helper/test-simple-ipc.c create mode 100755 t/t0052-simple-ipc.sh
base-commit: 71ca53e8125e36efbda17293c50027d31681a41f Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-766%2Fjeffhostetler%2Fsimple-ipc-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-766/jeffhostetler/simple-ipc-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/766
Range-diff vs v1:
1: 1155a45cf64 < -: ----------- pkt-line: use stack rather than static buffer in packet_write_gently()
-: ----------- > 1: 4c6766d4183 ci/install-depends: attempt to fix "brew cask" stuff
-: ----------- > 2: 3b03a8ff7a7 pkt-line: promote static buffer in packet_write_gently() to callers
-: ----------- > 3: e671894b4c0 pkt-line: add write_packetized_from_buf2() that takes scratch buffer
3: edf5ac95d66 ! 4: 0832f7d324d pkt-line: optionally skip the flush packet in write_packetized_from_buf()
@@ Commit message
packets before a final flush packet, so let's extend this function to
prepare for that scenario.
+ Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
## convert.c ##
@@ pkt-line.c: int write_packetized_from_fd(int fd_in, int fd_out)
-int write_packetized_from_buf(const char *src_in, size_t len, int fd_out)
+int write_packetized_from_buf(const char *src_in, size_t len, int fd_out,
+ int flush_at_end)
+ {
+ static struct packet_scratch_space scratch;
+
+- return write_packetized_from_buf2(src_in, len, fd_out, &scratch);
++ return write_packetized_from_buf2(src_in, len, fd_out,
++ flush_at_end, &scratch);
+ }
+
+ int write_packetized_from_buf2(const char *src_in, size_t len, int fd_out,
++ int flush_at_end,
+ struct packet_scratch_space *scratch)
{
int err = 0;
- size_t bytes_written = 0;
-@@ pkt-line.c: int write_packetized_from_buf(const char *src_in, size_t len, int fd_out)
- err = packet_write_gently(fd_out, src_in + bytes_written, bytes_to_write);
+@@ pkt-line.c: int write_packetized_from_buf2(const char *src_in, size_t len, int fd_out,
+ err = packet_write_gently(fd_out, src_in + bytes_written, bytes_to_write, scratch);
bytes_written += bytes_to_write;
}
- if (!err)
@@ pkt-line.h: void packet_buf_write_len(struct strbuf *buf, const char *data, size
-int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);
+int write_packetized_from_buf(const char *src_in, size_t len, int fd_out,
+ int flush_at_end);
+ int write_packetized_from_buf2(const char *src_in, size_t len, int fd_out,
++ int flush_at_end,
+ struct packet_scratch_space *scratch);
/*
- * Read a packetized line into the buffer, which must be at least size bytes
2: b7d678bc918 ! 5: 43bc4a26b79 pkt-line: (optionally) libify the packet readers
@@ pkt-line.c: enum packet_read_status packet_read_with_status(int fd, char **src_b
*pktlen = -1;
## pkt-line.h ##
-@@ pkt-line.h: int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);
+@@ pkt-line.h: int write_packetized_from_buf2(const char *src_in, size_t len, int fd_out,
*
* If options contains PACKET_READ_DIE_ON_ERR_PACKET, it dies when it sees an
* ERR packet.
4: 2f399ac107c = 6: 6a389a35335 pkt-line: accept additional options in read_packetized_to_strbuf()
5: 7064c5e9ffa ! 7: a7275b4bdc2 simple-ipc: design documentation for new IPC mechanism
@@ Documentation/technical/api-simple-ipc.txt (new)
+
+This IPC mechanism differs from the existing `sub-process.c` model
+(Documentation/technical/long-running-process-protocol.txt) and used
-+by applications like Git-LFS because the server is assumed to be very
-+long running system service. In contrast, a "sub-process model process"
-+is started with the foreground process and exits when the foreground
-+process terminates. How the server is started is also outside the
-+scope of the IPC mechanism.
++by applications like Git-LFS. In the simple-ipc model the server is
++assumed to be a very long-running system service. In contrast, in the
++LFS-style sub-process model the helper is started with the foreground
++process and exits when the foreground process terminates.
++
++How the simple-ipc server is started is also outside the scope of the
++IPC mechanism. For example, the server might be started during
++maintenance operations.
+
+The IPC protocol consists of a single request message from the client and
+an optional request message from the server. For simplicity, pkt-line
6: 9e27c07d785 ! 8: 388366913d4 simple-ipc: add win32 implementation
@@ compat/simple-ipc/ipc-win32.c (new)
+enum ipc_active_state ipc_client_try_connect(
+ const char *path,
+ const struct ipc_client_connect_options *options,
-+ int *pfd)
++ struct ipc_client_connection **p_connection)
+{
+ wchar_t wpath[MAX_PATH];
+ enum ipc_active_state state = IPC_STATE__OTHER_ERROR;
++ int fd = -1;
+
-+ *pfd = -1;
++ *p_connection = NULL;
+
+ trace2_region_enter("ipc-client", "try-connect", NULL);
+ trace2_data_string("ipc-client", NULL, "try-connect/path", path);
@@ compat/simple-ipc/ipc-win32.c (new)
+ state = IPC_STATE__INVALID_PATH;
+ else
+ state = connect_to_server(wpath, WINDOWS_CONNECTION_TIMEOUT_MS,
-+ options, pfd);
++ options, &fd);
+
+ trace2_data_intmax("ipc-client", NULL, "try-connect/state",
+ (intmax_t)state);
+ trace2_region_leave("ipc-client", "try-connect", NULL);
++
++ if (state == IPC_STATE__LISTENING) {
++ (*p_connection) = xcalloc(1, sizeof(struct ipc_client_connection));
++ (*p_connection)->fd = fd;
++ }
++
+ return state;
+}
+
-+int ipc_client_send_command_to_fd(int fd, const char *message,
-+ struct strbuf *answer)
++void ipc_client_close_connection(struct ipc_client_connection *connection)
++{
++ if (!connection)
++ return;
++
++ if (connection->fd != -1)
++ close(connection->fd);
++
++ free(connection);
++}
++
++int ipc_client_send_command_to_connection(
++ struct ipc_client_connection *connection,
++ const char *message, struct strbuf *answer)
+{
+ int ret = 0;
+
@@ compat/simple-ipc/ipc-win32.c (new)
+
+ trace2_region_enter("ipc-client", "send-command", NULL);
+
-+ if (write_packetized_from_buf(message, strlen(message), fd, 1) < 0) {
++ if (write_packetized_from_buf2(message, strlen(message),
++ connection->fd, 1,
++ &connection->scratch_write_buffer) < 0) {
+ ret = error(_("could not send IPC command"));
+ goto done;
+ }
+
-+ FlushFileBuffers((HANDLE)_get_osfhandle(fd));
++ FlushFileBuffers((HANDLE)_get_osfhandle(connection->fd));
+
-+ if (read_packetized_to_strbuf(fd, answer, PACKET_READ_NEVER_DIE) < 0) {
++ if (read_packetized_to_strbuf(connection->fd, answer,
++ PACKET_READ_NEVER_DIE) < 0) {
+ ret = error(_("could not read IPC response"));
+ goto done;
+ }
@@ compat/simple-ipc/ipc-win32.c (new)
+ const struct ipc_client_connect_options *options,
+ const char *message, struct strbuf *response)
+{
-+ int fd;
+ int ret = -1;
+ enum ipc_active_state state;
++ struct ipc_client_connection *connection = NULL;
+
-+ state = ipc_client_try_connect(path, options, &fd);
++ state = ipc_client_try_connect(path, options, &connection);
+
+ if (state != IPC_STATE__LISTENING)
+ return ret;
+
-+ ret = ipc_client_send_command_to_fd(fd, message, response);
-+ close(fd);
++ ret = ipc_client_send_command_to_connection(connection, message, response);
++
++ ipc_client_close_connection(connection);
++
+ return ret;
+}
+
@@ compat/simple-ipc/ipc-win32.c (new)
+ struct ipc_server_data *server_data;
+ pthread_t pthread_id;
+ HANDLE hPipe;
++ struct packet_scratch_space scratch_write_buffer;
+};
+
+/*
@@ compat/simple-ipc/ipc-win32.c (new)
+static int do_io_reply_callback(struct ipc_server_reply_data *reply_data,
+ const char *response, size_t response_len)
+{
++ struct packet_scratch_space *scratch =
++ &reply_data->server_thread_data->scratch_write_buffer;
++
+ if (reply_data->magic != MAGIC_SERVER_REPLY_DATA)
+ BUG("reply_cb called with wrong instance data");
+
-+ return write_packetized_from_buf(response, response_len,
-+ reply_data->fd, 0);
++ return write_packetized_from_buf2(response, response_len,
++ reply_data->fd, 0, scratch);
+}
+
+/*
@@ simple-ipc.h (new)
+#endif
+
+#ifdef SUPPORTS_SIMPLE_IPC
++#include "pkt-line.h"
+
+/*
+ * Simple IPC Client Side API.
@@ simple-ipc.h (new)
+ */
+enum ipc_active_state ipc_get_active_state(const char *path);
+
++struct ipc_client_connection {
++ int fd;
++ struct packet_scratch_space scratch_write_buffer;
++};
++
+/*
+ * Try to connect to the daemon on the named pipe or socket.
+ *
-+ * Returns IPC_STATE__LISTENING (and an fd) when connected.
++ * Returns IPC_STATE__LISTENING and a connection handle.
+ *
+ * Otherwise, returns info to help decide whether to retry or to
+ * spawn/respawn the server.
@@ simple-ipc.h (new)
+enum ipc_active_state ipc_client_try_connect(
+ const char *path,
+ const struct ipc_client_connect_options *options,
-+ int *pfd);
++ struct ipc_client_connection **p_connection);
++
++void ipc_client_close_connection(struct ipc_client_connection *connection);
+
+/*
+ * Used by the client to synchronously send and receive a message with
-+ * the server on the provided fd.
++ * the server on the provided client connection.
+ *
+ * Returns 0 when successful.
+ *
+ * Calls error() and returns non-zero otherwise.
+ */
-+int ipc_client_send_command_to_fd(int fd, const char *message,
-+ struct strbuf *answer);
++int ipc_client_send_command_to_connection(
++ struct ipc_client_connection *connection,
++ const char *message, struct strbuf *answer);
+
+/*
+ * Used by the client to synchronously connect and send and receive a
9: 69969c2b8d3 = 9: f0bebf1cdb3 simple-ipc: add t/helper/test-simple-ipc and t0052
-: ----------- > 10: f5d5445cf42 unix-socket: elimiate static unix_stream_socket() helper function
7: 96268351ac6 ! 11: 7a6a69dfc20 unix-socket: create gentle version of unix_stream_listen()
@@ Metadata
Author: Jeff Hostetler <jeffhost@microsoft.com>
## Commit message ##
- unix-socket: create gentle version of unix_stream_listen()
+ unix-socket: add options to unix_stream_listen()
- Create a gentle version of `unix_stream_listen()`. This version does
- not call `die()` if a socket-fd cannot be created and does not assume
- that it is safe to `unlink()` an existing socket-inode.
+ Update `unix_stream_listen()` to take an options structure to override
+ default behaviors. This includes the size of the `listen()` backlog
+ and whether it should always unlink the socket file before trying to
+ create a new one. Also eliminate calls to `die()` if it cannot create
+ a socket.
- `unix_stream_listen()` uses `unix_stream_socket()` helper function to
- create the socket-fd. Avoid that helper because it calls `die()` on
- errors.
-
- `unix_stream_listen()` always tries to `unlink()` the socket-path before
- calling `bind()`. If there is an existing server/daemon already bound
- and listening on that socket-path, our `unlink()` would have the effect
- of disassociating the existing server's bound-socket-fd from the socket-path
- without notifying the existing server. The existing server could continue
- to service existing connections (accepted-socket-fd's), but would not
- receive any futher new connections (since clients rendezvous via the
- socket-path). The existing server would effectively be offline but yet
- appear to be active.
+ Normally, `unix_stream_listen()` always tries to `unlink()` the
+ socket-path before calling `bind()`. If there is an existing
+ server/daemon already bound and listening on that socket-path, our
+ `unlink()` would have the effect of disassociating the existing
+ server's bound-socket-fd from the socket-path without notifying the
+ existing server. The existing server could continue to service
+ existing connections (accepted-socket-fd's), but would not receive any
+ futher new connections (since clients rendezvous via the socket-path).
+ The existing server would effectively be offline but yet appear to be
+ active.
Furthermore, `unix_stream_listen()` creates an opportunity for a brief
race condition for connecting clients if they try to connect in the
@@ Commit message
Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
+ ## builtin/credential-cache--daemon.c ##
+@@ builtin/credential-cache--daemon.c: static int serve_cache_loop(int fd)
+
+ static void serve_cache(const char *socket_path, int debug)
+ {
++ struct unix_stream_listen_opts opts = UNIX_STREAM_LISTEN_OPTS_INIT;
+ int fd;
+
+- fd = unix_stream_listen(socket_path);
++ fd = unix_stream_listen(socket_path, &opts);
+ if (fd < 0)
+ die_errno("unable to bind to '%s'", socket_path);
+
+
## unix-socket.c ##
-@@ unix-socket.c: int unix_stream_listen(const char *path)
- errno = saved_errno;
+@@ unix-socket.c: int unix_stream_connect(const char *path)
return -1;
}
-+
-+int unix_stream_listen_gently(const char *path,
-+ const struct unix_stream_listen_opts *opts)
-+{
+
+-int unix_stream_listen(const char *path)
++int unix_stream_listen(const char *path,
++ const struct unix_stream_listen_opts *opts)
+ {
+- int fd, saved_errno;
+ int fd = -1;
-+ int bind_successful = 0;
+ int saved_errno;
-+ struct sockaddr_un sa;
-+ struct unix_sockaddr_context ctx;
-+
-+ if (unix_sockaddr_init(&sa, path, &ctx) < 0)
-+ goto fail;
++ int bind_successful = 0;
++ int backlog;
+ struct sockaddr_un sa;
+ struct unix_sockaddr_context ctx;
+
+- unlink(path);
+-
+ if (unix_sockaddr_init(&sa, path, &ctx) < 0)
+ return -1;
+
-+ fd = socket(AF_UNIX, SOCK_STREAM, 0);
-+ if (fd < 0)
+ fd = socket(AF_UNIX, SOCK_STREAM, 0);
+ if (fd < 0)
+- die_errno("unable to create socket");
+ goto fail;
+
+ if (opts->force_unlink_before_bind)
+ unlink(path);
-+
-+ if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0)
-+ goto fail;
+
+ if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0)
+ goto fail;
+ bind_successful = 1;
-+
-+ if (listen(fd, opts->listen_backlog_size) < 0)
-+ goto fail;
-+
-+ unix_sockaddr_cleanup(&ctx);
-+ return fd;
-+
-+fail:
-+ saved_errno = errno;
-+ unix_sockaddr_cleanup(&ctx);
-+ close(fd);
+
+- if (listen(fd, 5) < 0)
++ if (opts->listen_backlog_size > 0)
++ backlog = opts->listen_backlog_size;
++ else
++ backlog = 5;
++ if (listen(fd, backlog) < 0)
+ goto fail;
+
+ unix_sockaddr_cleanup(&ctx);
+@@ unix-socket.c: int unix_stream_listen(const char *path)
+ fail:
+ saved_errno = errno;
+ unix_sockaddr_cleanup(&ctx);
+- close(fd);
++ if (fd != -1)
++ close(fd);
+ if (bind_successful)
+ unlink(path);
-+ errno = saved_errno;
-+ return -1;
-+}
+ errno = saved_errno;
+ return -1;
+ }
## unix-socket.h ##
@@
- int unix_stream_connect(const char *path);
- int unix_stream_listen(const char *path);
+ #ifndef UNIX_SOCKET_H
+ #define UNIX_SOCKET_H
+struct unix_stream_listen_opts {
+ int listen_backlog_size;
+ unsigned int force_unlink_before_bind:1;
+};
+
-+int unix_stream_listen_gently(const char *path,
-+ const struct unix_stream_listen_opts *opts);
++#define UNIX_STREAM_LISTEN_OPTS_INIT \
++{ \
++ .listen_backlog_size = 5, \
++ .force_unlink_before_bind = 1, \
++}
+
+ int unix_stream_connect(const char *path);
+-int unix_stream_listen(const char *path);
++int unix_stream_listen(const char *path,
++ const struct unix_stream_listen_opts *opts);
+
#endif /* UNIX_SOCKET_H */
8: 383a9755669 ! 12: 745b6d5fb74 unix-socket: add no-chdir option to unix_stream_listen_gently()
@@ Metadata
Author: Jeff Hostetler <jeffhost@microsoft.com>
## Commit message ##
- unix-socket: add no-chdir option to unix_stream_listen_gently()
+ unix-socket: add no-chdir option to unix_stream_listen()
Calls to `chdir()` are dangerous in a multi-threaded context. If
`unix_stream_listen()` is given a socket pathname that is too big to
@@ Commit message
Teach `unix_sockaddr_init()` to not allow calls to `chdir()` when flag
is set.
- Extend the public interface to `unix_stream_listen_gently()` to also
- expose this new flag.
-
Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
## unix-socket.c ##
@@ unix-socket.c: int unix_stream_connect(const char *path)
if (unix_sockaddr_init(&sa, path, &ctx) < 0)
return -1;
-@@ unix-socket.c: int unix_stream_listen(const char *path)
- {
- int fd, saved_errno;
- struct sockaddr_un sa;
-- struct unix_sockaddr_context ctx;
-+ struct unix_sockaddr_context ctx = UNIX_SOCKADDR_CONTEXT_INIT;
-
- unlink(path);
-
-@@ unix-socket.c: int unix_stream_listen_gently(const char *path,
+@@ unix-socket.c: int unix_stream_listen(const char *path,
int bind_successful = 0;
- int saved_errno;
+ int backlog;
struct sockaddr_un sa;
- struct unix_sockaddr_context ctx;
+ struct unix_sockaddr_context ctx = UNIX_SOCKADDR_CONTEXT_INIT;
@@ unix-socket.c: int unix_stream_listen_gently(const char *path,
+ ctx.disallow_chdir = opts->disallow_chdir;
if (unix_sockaddr_init(&sa, path, &ctx) < 0)
- goto fail;
+ return -1;
## unix-socket.h ##
-@@ unix-socket.h: int unix_stream_listen(const char *path);
+@@
struct unix_stream_listen_opts {
int listen_backlog_size;
unsigned int force_unlink_before_bind:1;
+ unsigned int disallow_chdir:1;
};
- int unix_stream_listen_gently(const char *path,
+ #define UNIX_STREAM_LISTEN_OPTS_INIT \
+ { \
+ .listen_backlog_size = 5, \
+ .force_unlink_before_bind = 1, \
++ .disallow_chdir = 0, \
+ }
+
+ int unix_stream_connect(const char *path);
-: ----------- > 13: 2cca15a10ec unix-socket: do not call die in unix_stream_connect()
10: a1b15fb5cb0 ! 14: 72c1c209c38 simple-ipc: add Unix domain socket implementation
@@ Commit message
Create Unix domain socket based implementation of "simple-ipc".
+ A set of `ipc_client` routines implement a client library to connect
+ to an `ipc_server` over a Unix domain socket, send a simple request,
+ and receive a single response. Clients use blocking IO on the socket.
+
+ A set of `ipc_server` routines implement a thread pool to listen for
+ and concurrently service client connections.
+
+ The server creates a new Unix domain socket at a known location. If a
+ socket already exists with that name, the server tries to determine if
+ another server is already listening on the socket or if the socket is
+ dead. If socket is busy, the server exits with an error rather than
+ stealing the socket. If the socket is dead, the server creates a new
+ one and starts up.
+
+ If while running, the server detects that its socket has been stolen
+ by another server, it automatically exits.
+
Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
## Makefile ##
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+ struct ipc_client_connect_options options
+ = IPC_CLIENT_CONNECT_OPTIONS_INIT;
+ struct stat st;
-+ int fd_test = -1;
++ struct ipc_client_connection *connection_test = NULL;
+
+ options.wait_if_busy = 0;
+ options.wait_if_not_found = 0;
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+ * at `path`, doesn't mean it that there is a server listening.
+ * Ping it to be sure.
+ */
-+ state = ipc_client_try_connect(path, &options, &fd_test);
-+ close(fd_test);
++ state = ipc_client_try_connect(path, &options, &connection_test);
++ ipc_client_close_connection(connection_test);
+
+ return state;
+}
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+enum ipc_active_state ipc_client_try_connect(
+ const char *path,
+ const struct ipc_client_connect_options *options,
-+ int *pfd)
++ struct ipc_client_connection **p_connection)
+{
+ enum ipc_active_state state = IPC_STATE__OTHER_ERROR;
++ int fd = -1;
+
-+ *pfd = -1;
++ *p_connection = NULL;
+
+ trace2_region_enter("ipc-client", "try-connect", NULL);
+ trace2_data_string("ipc-client", NULL, "try-connect/path", path);
+
+ state = connect_to_server(path, MY_CONNECTION_TIMEOUT_MS,
-+ options, pfd);
++ options, &fd);
+
+ trace2_data_intmax("ipc-client", NULL, "try-connect/state",
+ (intmax_t)state);
+ trace2_region_leave("ipc-client", "try-connect", NULL);
++
++ if (state == IPC_STATE__LISTENING) {
++ (*p_connection) = xcalloc(1, sizeof(struct ipc_client_connection));
++ (*p_connection)->fd = fd;
++ }
++
+ return state;
+}
+
-+int ipc_client_send_command_to_fd(int fd, const char *message,
-+ struct strbuf *answer)
++void ipc_client_close_connection(struct ipc_client_connection *connection)
++{
++ if (!connection)
++ return;
++
++ if (connection->fd != -1)
++ close(connection->fd);
++
++ free(connection);
++}
++
++int ipc_client_send_command_to_connection(
++ struct ipc_client_connection *connection,
++ const char *message, struct strbuf *answer)
+{
+ int ret = 0;
+
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+
+ trace2_region_enter("ipc-client", "send-command", NULL);
+
-+ if (write_packetized_from_buf(message, strlen(message), fd, 1) < 0) {
++ if (write_packetized_from_buf2(message, strlen(message),
++ connection->fd, 1,
++ &connection->scratch_write_buffer) < 0) {
+ ret = error(_("could not send IPC command"));
+ goto done;
+ }
+
-+ if (read_packetized_to_strbuf(fd, answer, PACKET_READ_NEVER_DIE) < 0) {
++ if (read_packetized_to_strbuf(connection->fd, answer,
++ PACKET_READ_NEVER_DIE) < 0) {
+ ret = error(_("could not read IPC response"));
+ goto done;
+ }
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+ const struct ipc_client_connect_options *options,
+ const char *message, struct strbuf *answer)
+{
-+ int fd;
+ int ret = -1;
+ enum ipc_active_state state;
++ struct ipc_client_connection *connection = NULL;
+
-+ state = ipc_client_try_connect(path, options, &fd);
++ state = ipc_client_try_connect(path, options, &connection);
+
+ if (state != IPC_STATE__LISTENING)
+ return ret;
+
-+ ret = ipc_client_send_command_to_fd(fd, message, answer);
-+ close(fd);
++ ret = ipc_client_send_command_to_connection(connection, message, answer);
++
++ ipc_client_close_connection(connection);
++
+ return ret;
+}
+
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+ struct ipc_worker_thread_data *next_thread;
+ struct ipc_server_data *server_data;
+ pthread_t pthread_id;
++ struct packet_scratch_space scratch_write_buffer;
+};
+
+struct ipc_accept_thread_data {
+ enum magic magic;
+ struct ipc_server_data *server_data;
++
+ int fd_listen;
-+ ino_t inode_listen;
++ struct stat st_listen;
++
+ int fd_send_shutdown;
+ int fd_wait_shutdown;
+ pthread_t pthread_id;
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+static int do_io_reply_callback(struct ipc_server_reply_data *reply_data,
+ const char *response, size_t response_len)
+{
++ struct packet_scratch_space *scratch =
++ &reply_data->worker_thread_data->scratch_write_buffer;
++
+ if (reply_data->magic != MAGIC_SERVER_REPLY_DATA)
+ BUG("reply_cb called with wrong instance data");
+
-+ return write_packetized_from_buf(response, response_len,
-+ reply_data->fd, 0);
++ return write_packetized_from_buf2(response, response_len,
++ reply_data->fd, 0, scratch);
+}
+
+/* A randomly chosen value. */
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+static int socket_was_stolen(struct ipc_accept_thread_data *accept_thread_data)
+{
+ struct stat st;
++ struct stat *ref_st = &accept_thread_data->st_listen;
+
+ if (lstat(accept_thread_data->server_data->buf_path.buf, &st) == -1)
+ return 1;
+
-+ if (st.st_ino != accept_thread_data->inode_listen)
++ if (st.st_ino != ref_st->st_ino)
+ return 1;
+
++ /* We might also consider the creation time on some platforms. */
++
+ return 0;
+}
+
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+ * open/connect using the "socket-inode" pathname.
+ *
+ * Unix domain sockets have a fundamental design flaw because the
-+ * "socket-inode" persists until the pathname is deleted; closing the listening
-+ * "socket-fd" only closes the socket handle/descriptor, it does not delete
-+ * the inode/pathname.
++ * "socket-inode" persists until the pathname is deleted; closing the
++ * listening "socket-fd" only closes the socket handle/descriptor, it
++ * does not delete the inode/pathname.
+ *
+ * Well-behaving service daemons are expected to also delete the inode
+ * before shutdown. If a service crashes (or forgets) it can leave
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+ * inode *or* another service instance is already running.
+ *
+ * One possible solution is to blindly unlink the inode before
-+ * attempting to bind a new socket-fd (and thus create) a new
++ * attempting to bind a new socket-fd and thus create a new
+ * socket-inode. Then `bind(2)` should always succeed. However, if
-+ * there is an existing service instance, it would be orphaned --
-+ * it would still be listening on a socket-fd that is still bound
-+ * to an (unlinked) socket-inode, but that socket-inode is no longer
++ * there is an existing service instance, it would be orphaned -- it
++ * would still be listening on a socket-fd that is still bound to an
++ * (unlinked) socket-inode, but that socket-inode is no longer
+ * associated with the pathname. New client connections will arrive
-+ * at our new socket-inode and not the existing server's. (It is upto
-+ * the existing server to detect that its socket-inode has been
-+ * stolen and shutdown.)
++ * at OUR new socket-inode -- rather than the existing server's
++ * socket. (I suppose it is up to the existing server to detect that
++ * its socket-inode has been stolen and shutdown.)
++ *
++ * Another possible solution is to try to use the ".lock" trick, but
++ * bind() does not have a exclusive-create use bit like open() does,
++ * so we cannot have multiple servers fighting/racing to create the
++ * same file name without having losers lose without knowing that they
++ * lost.
++ *
++ * We try to avoid such stealing and would rather fail to run than
++ * steal an existing socket-inode (because we assume that the
++ * existing server has more context and value to the clients than a
++ * freshly started server). However, if multiple servers are racing
++ * to start, we don't care which one wins -- none of them have any
++ * state information yet worth fighting for.
++ *
++ * Create a "unique" socket-inode (with our PID in it (and assume that
++ * we can force-delete an existing socket with that name)). Stat it
++ * to get the inode number and ctime -- so that we can identify it as
++ * the one we created. Then use the atomic-rename trick to install it
++ * in the real location. (This will unlink an existing socket with
++ * that pathname -- and thereby steal the real socket-inode from an
++ * existing server.)
+ *
-+ * Since this is rather obscure and infrequent, we try to "gently"
-+ * create the socket-inode without disturbing an existing service.
++ * Elsewhere, our thread will periodically poll the socket-inode to
++ * see if someone else steals ours.
+ */
+static int create_listener_socket(const char *path,
-+ const struct ipc_server_opts *ipc_opts)
++ const struct ipc_server_opts *ipc_opts,
++ struct stat *st_socket)
+{
++ struct stat st;
++ struct strbuf buf_uniq = STRBUF_INIT;
+ int fd_listen;
-+ int fd_client;
-+ struct unix_stream_listen_opts uslg_opts = {
-+ .listen_backlog_size = LISTEN_BACKLOG,
-+ .force_unlink_before_bind = 0,
-+ .disallow_chdir = ipc_opts->uds_disallow_chdir
-+ };
-+
-+ trace2_data_string("ipc-server", NULL, "try-listen-gently", path);
-+
-+ /*
-+ * Assume socket-inode does not exist and try to (gently)
-+ * create a new socket-inode on disk at pathname and bind
-+ * socket-fd to it.
-+ */
-+ fd_listen = unix_stream_listen_gently(path, &uslg_opts);
-+ if (fd_listen >= 0)
-+ return fd_listen;
++ struct unix_stream_listen_opts uslg_opts = UNIX_STREAM_LISTEN_OPTS_INIT;
+
-+ if (errno != EADDRINUSE)
-+ return error_errno(_("could not create socket '%s'"),
-+ path);
-+
-+ trace2_data_string("ipc-server", NULL, "try-detect-server", path);
-+
-+ /*
-+ * A socket-inode at pathname exists on disk, but we don't
-+ * know if it a server is using it or if it is a stale inode.
-+ *
-+ * poke it with a trivial connection to try to find out.
-+ */
-+ fd_client = unix_stream_connect(path);
-+ if (fd_client >= 0) {
++ if (!lstat(path, &st) && S_ISSOCK(st.st_mode)) {
++ int fd_client;
+ /*
-+ * An existing service process is alive and accepted our
-+ * connection.
++ * A socket-inode at `path` exists on disk, but we
++ * don't know whether it belongs to an active server
++ * or if the last server died without cleaning up.
++ *
++ * Poke it with a trivial connection to try to find out.
+ */
-+ close(fd_client);
-+
-+ /*
-+ * We cannot create a new socket-inode here, so we cannot
-+ * startup a new server on this pathname.
-+ */
-+ errno = EADDRINUSE;
-+ return error_errno(_("socket already in use '%s'"),
++ trace2_data_string("ipc-server", NULL, "try-detect-server",
+ path);
++ fd_client = unix_stream_connect(path);
++ if (fd_client >= 0) {
++ close(fd_client);
++ errno = EADDRINUSE;
++ return error_errno(_("socket already in use '%s'"),
++ path);
++ }
+ }
+
-+ trace2_data_string("ipc-server", NULL, "try-listen-force", path);
-+
+ /*
-+ * A socket-inode at pathname exists on disk, but we were not
-+ * able to connect to it, so we believe that this is a stale
-+ * socket-inode that a previous server forgot to delete. Use
-+ * the tradional solution: force unlink it and create a new
-+ * one.
-+ *
-+ * TODO Note that it is possible that another server is
-+ * listening, but is either just starting up and not yet
-+ * responsive or is stuck somehow. For now, I'm OK with
-+ * stealing the socket-inode from it in this case.
++ * Create pathname to our "unique" socket and set it up for
++ * business.
+ */
-+ uslg_opts.force_unlink_before_bind = 1;
-+ fd_listen = unix_stream_listen_gently(path, &uslg_opts);
-+ if (fd_listen >= 0)
-+ return fd_listen;
-+
-+ return error_errno(_("could not force create socket '%s'"), path);
-+}
-+
-+static int setup_listener_socket(const char *path, ino_t *inode,
-+ const struct ipc_server_opts *ipc_opts)
-+{
-+ int fd_listen;
-+ struct stat st;
-+
-+ trace2_region_enter("ipc-server", "create-listener_socket", NULL);
-+ fd_listen = create_listener_socket(path, ipc_opts);
-+ trace2_region_leave("ipc-server", "create-listener_socket", NULL);
++ strbuf_addf(&buf_uniq, "%s.%d", path, getpid());
+
-+ if (fd_listen < 0)
-+ return fd_listen;
-+
-+ /*
-+ * We just bound a socket (descriptor) to a newly created unix
-+ * domain socket in the filesystem. Capture the inode number
-+ * so we can later detect if/when someone else force-creates a
-+ * new socket and effectively steals the path from us. (Which
-+ * would leave us listening to a socket that no client could
-+ * reach.)
-+ */
-+ if (lstat(path, &st) < 0) {
++ uslg_opts.listen_backlog_size = LISTEN_BACKLOG;
++ uslg_opts.force_unlink_before_bind = 1;
++ uslg_opts.disallow_chdir = ipc_opts->uds_disallow_chdir;
++ fd_listen = unix_stream_listen(buf_uniq.buf, &uslg_opts);
++ if (fd_listen < 0) {
+ int saved_errno = errno;
++ error_errno(_("could not create listener socket '%s'"),
++ buf_uniq.buf);
++ strbuf_release(&buf_uniq);
++ errno = saved_errno;
++ return -1;
++ }
+
++ if (lstat(buf_uniq.buf, st_socket)) {
++ int saved_errno = errno;
++ error_errno(_("could not stat listener socket '%s'"),
++ buf_uniq.buf);
+ close(fd_listen);
-+ unlink(path);
-+
++ unlink(buf_uniq.buf);
++ strbuf_release(&buf_uniq);
+ errno = saved_errno;
-+ return error_errno(_("could not lstat listener socket '%s'"),
-+ path);
++ return -1;
+ }
+
+ if (set_socket_blocking_flag(fd_listen, 1)) {
+ int saved_errno = errno;
-+
++ error_errno(_("could not set listener socket nonblocking '%s'"),
++ buf_uniq.buf);
+ close(fd_listen);
-+ unlink(path);
++ unlink(buf_uniq.buf);
++ strbuf_release(&buf_uniq);
++ errno = saved_errno;
++ return -1;
++ }
+
++ /*
++ * Install it as the "real" socket so that clients will starting
++ * connecting to our socket.
++ */
++ if (rename(buf_uniq.buf, path)) {
++ int saved_errno = errno;
++ error_errno(_("could not create listener socket '%s'"), path);
++ close(fd_listen);
++ unlink(buf_uniq.buf);
++ strbuf_release(&buf_uniq);
+ errno = saved_errno;
-+ return error_errno(_("making listener socket nonblocking '%s'"),
-+ path);
++ return -1;
+ }
+
-+ *inode = st.st_ino;
++ strbuf_release(&buf_uniq);
++ trace2_data_string("ipc-server", NULL, "try-listen", path);
++ return fd_listen;
++}
++
++static int setup_listener_socket(const char *path, struct stat *st_socket,
++ const struct ipc_server_opts *ipc_opts)
++{
++ int fd_listen;
++
++ trace2_region_enter("ipc-server", "create-listener_socket", NULL);
++ fd_listen = create_listener_socket(path, ipc_opts, st_socket);
++ trace2_region_leave("ipc-server", "create-listener_socket", NULL);
+
+ return fd_listen;
+}
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+{
+ struct ipc_server_data *server_data;
+ int fd_listen;
-+ ino_t inode_listen;
++ struct stat st_listen;
+ int sv[2];
+ int k;
+ int nr_threads = opts->nr_threads;
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+ path);
+ }
+
-+ fd_listen = setup_listener_socket(path, &inode_listen, opts);
++ fd_listen = setup_listener_socket(path, &st_listen, opts);
+ if (fd_listen < 0) {
+ int saved_errno = errno;
+ close(sv[0]);
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+ server_data->accept_thread->magic = MAGIC_ACCEPT_THREAD_DATA;
+ server_data->accept_thread->server_data = server_data;
+ server_data->accept_thread->fd_listen = fd_listen;
-+ server_data->accept_thread->inode_listen = inode_listen;
++ server_data->accept_thread->st_listen = st_listen;
+ server_data->accept_thread->fd_send_shutdown = sv[0];
+ server_data->accept_thread->fd_wait_shutdown = sv[1];
+-- gitgitgadget