[v6,00/12] Simple IPC Mechanism

Message

Philippe Blain via GitGitGadget March 15, 2021, 9:08 p.m. UTC

Here is V6 of my "Simple IPC" series. This version addresses comments from
last week on V5. This includes:

 1. Removing "perf and" in pkt-line.c
 2. Better comments to describe the various timeout #define's.
 3. Remove the double-underscore and shorten the "unix_stream_server"
    prefix.

Thanks Jeff

Jeff Hostetler (9):
  pkt-line: eliminate the need for static buffer in
    packet_write_gently()
  simple-ipc: design documentation for new IPC mechanism
  simple-ipc: add win32 implementation
  unix-socket: eliminate static unix_stream_socket() helper function
  unix-socket: add backlog size option to unix_stream_listen()
  unix-socket: disallow chdir() when creating unix domain sockets
  unix-stream-server: create unix domain socket under lock
  simple-ipc: add Unix domain socket implementation
  t0052: add simple-ipc tests and t/helper/test-simple-ipc tool

Johannes Schindelin (3):
  pkt-line: do not issue flush packets in write_packetized_*()
  pkt-line: add PACKET_READ_GENTLE_ON_READ_ERROR option
  pkt-line: add options argument to read_packetized_to_strbuf()

 Documentation/technical/api-simple-ipc.txt |  105 ++
 Makefile                                   |    9 +
 builtin/credential-cache--daemon.c         |    3 +-
 builtin/credential-cache.c                 |    2 +-
 compat/simple-ipc/ipc-shared.c             |   28 +
 compat/simple-ipc/ipc-unix-socket.c        | 1000 ++++++++++++++++++++
 compat/simple-ipc/ipc-win32.c              |  751 +++++++++++++++
 config.mak.uname                           |    2 +
 contrib/buildsystems/CMakeLists.txt        |    8 +-
 convert.c                                  |   11 +-
 pkt-line.c                                 |   59 +-
 pkt-line.h                                 |   17 +-
 simple-ipc.h                               |  239 +++++
 t/helper/test-simple-ipc.c                 |  787 +++++++++++++++
 t/helper/test-tool.c                       |    1 +
 t/helper/test-tool.h                       |    1 +
 t/t0052-simple-ipc.sh                      |  122 +++
 unix-socket.c                              |   53 +-
 unix-socket.h                              |   12 +-
 unix-stream-server.c                       |  125 +++
 unix-stream-server.h                       |   33 +
 21 files changed, 3316 insertions(+), 52 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
 create mode 100644 unix-stream-server.c
 create mode 100644 unix-stream-server.h


base-commit: f01623b2c9d14207e497b21ebc6b3ec4afaf4b46
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-766%2Fjeffhostetler%2Fsimple-ipc-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-766/jeffhostetler/simple-ipc-v6
Pull-Request: https://github.com/gitgitgadget/git/pull/766

Range-diff vs v5:

  1:  311ea4a5cd71 !  1:  fe35dc3d292d pkt-line: eliminate the need for static buffer in packet_write_gently()
     @@ pkt-line.c: int packet_write_fmt_gently(int fd, const char *fmt, ...)
      +	set_packet_header(header, packet_size);
      +
      +	/*
     -+	 * Write the header and the buffer in 2 parts so that we do not need
     -+	 * to allocate a buffer or rely on a static buffer.  This avoids perf
     -+	 * and multi-threading issues.
     ++	 * Write the header and the buffer in 2 parts so that we do
     ++	 * not need to allocate a buffer or rely on a static buffer.
     ++	 * This also avoids putting a large buffer on the stack which
     ++	 * might have multi-threading issues.
      +	 */
      +
      +	if (write_in_full(fd_out, header, 4) < 0 ||
  2:  25157c1f4873 =  2:  de11b3036148 pkt-line: do not issue flush packets in write_packetized_*()
  3:  af3d13113bc9 =  3:  3718da39da30 pkt-line: add PACKET_READ_GENTLE_ON_READ_ERROR option
  4:  b73e66a69b61 =  4:  b43df7ad0b7a pkt-line: add options argument to read_packetized_to_strbuf()
  5:  1ae99d824a21 =  5:  f829feb2aa93 simple-ipc: design documentation for new IPC mechanism
  6:  8b3ce40e4538 =  6:  58c3fb7cd776 simple-ipc: add win32 implementation
  7:  34df1af98e5b =  7:  4e8c352fb366 unix-socket: eliminate static unix_stream_socket() helper function
  8:  d6ff6e0e050a =  8:  3b71f52d8628 unix-socket: add backlog size option to unix_stream_listen()
  9:  21b8d3c63dbf =  9:  5972a198361c unix-socket: disallow chdir() when creating unix domain sockets
 10:  1ee9de55a106 ! 10:  02c885fd623d unix-stream-server: create unix domain socket under lock
     @@ unix-stream-server.c (new)
      +	return 0;
      +}
      +
     -+int unix_stream_server__create(
     -+	const char *path,
     -+	const struct unix_stream_listen_opts *opts,
     -+	long timeout_ms,
     -+	struct unix_stream_server_socket **new_server_socket)
     ++int unix_ss_create(const char *path,
     ++		   const struct unix_stream_listen_opts *opts,
     ++		   long timeout_ms,
     ++		   struct unix_ss_socket **new_server_socket)
      +{
      +	struct lock_file lock = LOCK_INIT;
      +	int fd_socket;
     -+	struct unix_stream_server_socket *server_socket;
     ++	struct unix_ss_socket *server_socket;
      +
      +	*new_server_socket = NULL;
      +
     @@ unix-stream-server.c (new)
      +	return 0;
      +}
      +
     -+void unix_stream_server__free(
     -+	struct unix_stream_server_socket *server_socket)
     ++void unix_ss_free(struct unix_ss_socket *server_socket)
      +{
      +	if (!server_socket)
      +		return;
      +
      +	if (server_socket->fd_socket >= 0) {
     -+		if (!unix_stream_server__was_stolen(server_socket))
     ++		if (!unix_ss_was_stolen(server_socket))
      +			unlink(server_socket->path_socket);
      +		close(server_socket->fd_socket);
      +	}
     @@ unix-stream-server.c (new)
      +	free(server_socket);
      +}
      +
     -+int unix_stream_server__was_stolen(
     -+	struct unix_stream_server_socket *server_socket)
     ++int unix_ss_was_stolen(struct unix_ss_socket *server_socket)
      +{
      +	struct stat st_now;
      +
     @@ unix-stream-server.h (new)
      +
      +#include "unix-socket.h"
      +
     -+struct unix_stream_server_socket {
     ++struct unix_ss_socket {
      +	char *path_socket;
      +	struct stat st_socket;
      +	int fd_socket;
     @@ unix-stream-server.h (new)
      + *
      + * Returns 0 on success, -1 on error, -2 if socket is in use.
      + */
     -+int unix_stream_server__create(
     -+	const char *path,
     -+	const struct unix_stream_listen_opts *opts,
     -+	long timeout_ms,
     -+	struct unix_stream_server_socket **server_socket);
     ++int unix_ss_create(const char *path,
     ++		   const struct unix_stream_listen_opts *opts,
     ++		   long timeout_ms,
     ++		   struct unix_ss_socket **server_socket);
      +
      +/*
      + * Close and delete the socket.
      + */
     -+void unix_stream_server__free(
     -+	struct unix_stream_server_socket *server_socket);
     ++void unix_ss_free(struct unix_ss_socket *server_socket);
      +
      +/*
      + * Return 1 if the inode of the pathname to our socket changes.
      + */
     -+int unix_stream_server__was_stolen(
     -+	struct unix_stream_server_socket *server_socket);
     ++int unix_ss_was_stolen(struct unix_ss_socket *server_socket);
      +
      +#endif /* UNIX_STREAM_SERVER_H */
 11:  f2e3b046cc8f ! 11:  4c2199231d05 simple-ipc: add Unix domain socket implementation
     @@ compat/simple-ipc/ipc-unix-socket.c (new)
      +}
      +
      +/*
     -+ * This value was chosen at random.
     ++ * Retry frequency when trying to connect to a server.
     ++ *
     ++ * This value should be short enough that we don't seriously delay our
     ++ * caller, but not fast enough that our spinning puts pressure on the
     ++ * system.
      + */
      +#define WAIT_STEP_MS (50)
      +
     @@ compat/simple-ipc/ipc-unix-socket.c (new)
      +	const struct ipc_client_connect_options *options,
      +	int *pfd)
      +{
     -+	int wait_ms = 50;
      +	int k;
      +
      +	*pfd = -1;
      +
     -+	for (k = 0; k < timeout_ms; k += wait_ms) {
     ++	for (k = 0; k < timeout_ms; k += WAIT_STEP_MS) {
      +		int fd = unix_stream_connect(path, options->uds_disallow_chdir);
      +
      +		if (fd != -1) {
     @@ compat/simple-ipc/ipc-unix-socket.c (new)
      +		return IPC_STATE__OTHER_ERROR;
      +
      +	sleep_and_try_again:
     -+		sleep_millisec(wait_ms);
     ++		sleep_millisec(WAIT_STEP_MS);
      +	}
      +
      +	return IPC_STATE__NOT_LISTENING;
      +}
      +
      +/*
     -+ * A randomly chosen timeout value.
     ++ * The total amount of time that we are willing to wait when trying to
     ++ * connect to a server.
     ++ *
     ++ * When the server is first started, it might take a little while for
     ++ * it to become ready to service requests.  Likewise, the server may
     ++ * be very (temporarily) busy and not respond to our connections.
     ++ *
     ++ * We should gracefully and silently handle those conditions and try
     ++ * again for a reasonable time period.
     ++ *
     ++ * The value chosen here should be long enough for the server
     ++ * to reliably heal from the above conditions.
      + */
      +#define MY_CONNECTION_TIMEOUT_MS (1000)
      +
     @@ compat/simple-ipc/ipc-unix-socket.c (new)
      +	enum magic magic;
      +	struct ipc_server_data *server_data;
      +
     -+	struct unix_stream_server_socket *server_socket;
     ++	struct unix_ss_socket *server_socket;
      +
      +	int fd_send_shutdown;
      +	int fd_wait_shutdown;
     @@ compat/simple-ipc/ipc-unix-socket.c (new)
      +			 * will be routed elsewhere and we silently starve.
      +			 * If that happens, just queue a shutdown.
      +			 */
     -+			if (unix_stream_server__was_stolen(
     ++			if (unix_ss_was_stolen(
      +				    accept_thread_data->server_socket)) {
      +				trace2_data_string("ipc-accept", NULL,
      +						   "queue_stop_async",
     @@ compat/simple-ipc/ipc-unix-socket.c (new)
      +static int create_listener_socket(
      +	const char *path,
      +	const struct ipc_server_opts *ipc_opts,
     -+	struct unix_stream_server_socket **new_server_socket)
     ++	struct unix_ss_socket **new_server_socket)
      +{
     -+	struct unix_stream_server_socket *server_socket = NULL;
     ++	struct unix_ss_socket *server_socket = NULL;
      +	struct unix_stream_listen_opts uslg_opts = UNIX_STREAM_LISTEN_OPTS_INIT;
      +	int ret;
      +
      +	uslg_opts.listen_backlog_size = LISTEN_BACKLOG;
      +	uslg_opts.disallow_chdir = ipc_opts->uds_disallow_chdir;
      +
     -+	ret = unix_stream_server__create(path, &uslg_opts, -1, &server_socket);
     ++	ret = unix_ss_create(path, &uslg_opts, -1, &server_socket);
      +	if (ret)
      +		return ret;
      +
      +	if (set_socket_blocking_flag(server_socket->fd_socket, 1)) {
      +		int saved_errno = errno;
     -+		unix_stream_server__free(server_socket);
     ++		unix_ss_free(server_socket);
      +		errno = saved_errno;
      +		return -1;
      +	}
     @@ compat/simple-ipc/ipc-unix-socket.c (new)
      +static int setup_listener_socket(
      +	const char *path,
      +	const struct ipc_server_opts *ipc_opts,
     -+	struct unix_stream_server_socket **new_server_socket)
     ++	struct unix_ss_socket **new_server_socket)
      +{
      +	int ret, saved_errno;
      +
     @@ compat/simple-ipc/ipc-unix-socket.c (new)
      +			 ipc_server_application_cb *application_cb,
      +			 void *application_data)
      +{
     -+	struct unix_stream_server_socket *server_socket = NULL;
     ++	struct unix_ss_socket *server_socket = NULL;
      +	struct ipc_server_data *server_data;
      +	int sv[2];
      +	int k;
     @@ compat/simple-ipc/ipc-unix-socket.c (new)
      +
      +	accept_thread_data = server_data->accept_thread;
      +	if (accept_thread_data) {
     -+		unix_stream_server__free(accept_thread_data->server_socket);
     ++		unix_ss_free(accept_thread_data->server_socket);
      +
      +		if (accept_thread_data->fd_send_shutdown != -1)
      +			close(accept_thread_data->fd_send_shutdown);
 12:  6ccc7472096f = 12:  132b6f3271be t0052: add simple-ipc tests and t/helper/test-simple-ipc tool