Simple IPC Cleanups #893
Closed
Simple IPC Cleanups #893
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Teach `packet_write_gently()` to write the pkt-line header and the actual buffer in 2 separate calls to `write_in_full()` and avoid the need for a static buffer, thread-safe scratch space, or an excessively large stack buffer. Change the API of `write_packetized_from_fd()` to accept a scratch space argument from its caller to avoid similar issues here. These changes are intended to make it easier to use pkt-line routines in a multi-threaded context with multiple concurrent writers writing to different streams. Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Remove the `packet_flush_gently()` call in `write_packetized_from_buf() and `write_packetized_from_fd()` and require the caller to call it if desired. Rename both functions to `write_packetized_from_*_no_flush()` to prevent later merge accidents. `write_packetized_from_buf()` currently only has one caller: `apply_multi_file_filter()` in `convert.c`. It always wants a flush packet to be written after writing the payload. However, we are about to introduce a caller that wants to write many packets before a final flush packet, so let's make the caller responsible for emitting the flush packet. Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
So far, the (possibly indirect) callers of `get_packet_data()` can ask that function to return an error instead of `die()`ing upon end-of-file. However, random read errors will still cause the process to die. So let's introduce an explicit option to tell the packet reader machinery to please be nice and only return an error. This change prepares pkt-line for use by long-running daemon processes. Such processes should be able to serve multiple concurrent clients and and survive random IO errors. If there is an error on one connection, a daemon should be able to drop that connection and continue serving existing and future connections. This ability will be used by a Git-aware "Internal FSMonitor" feature in a later patch series. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Update the calling sequence of `read_packetized_to_strbuf()` to take an options argument and not assume a fixed set of options. Update the only existing caller accordingly to explicitly pass the formerly-assumed flags. The `read_packetized_to_strbuf()` function calls `packet_read()` with a fixed set of assumed options (`PACKET_READ_GENTLE_ON_EOF`). This assumption has been fine for the single existing caller `apply_multi_file_filter()` in `convert.c`. In a later commit we would like to add other callers to `read_packetized_to_strbuf()` that need a different set of options. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Brief design documentation for new IPC mechanism allowing foreground Git client to talk with an existing daemon process at a known location using a named pipe or unix domain socket. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Create Windows implementation of "simple-ipc" using named pipes. Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The static helper function `unix_stream_socket()` calls `die()`. This is not appropriate for all callers. Eliminate the wrapper function and make the callers propagate the error. Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Update `unix_stream_listen()` to take an options structure to override default behaviors. This commit includes the size of the `listen()` backlog. Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Calls to `chdir()` are dangerous in a multi-threaded context. If `unix_stream_listen()` or `unix_stream_connect()` is given a socket pathname that is too long to fit in a `sockaddr_un` structure, it will `chdir()` to the parent directory of the requested socket pathname, create the socket using a relative pathname, and then `chdir()` back. This is not thread-safe. Teach `unix_sockaddr_init()` to not allow calls to `chdir()` when this flag is set. Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Create a version of `unix_stream_listen()` that uses a ".lock" lockfile
to create the unix domain socket in a race-free manner.
Unix domain sockets have a fundamental problem on Unix systems because
they persist in the filesystem until they are deleted. This is
independent of whether a server is actually listening for connections.
Well-behaved servers are expected to delete the socket when they
shutdown. A new server cannot easily tell if a found socket is
attached to an active server or is leftover cruft from a dead server.
The traditional solution used by `unix_stream_listen()` is to force
delete the socket pathname and then create a new socket. This solves
the latter (cruft) problem, but in the case of the former, it orphans
the existing server (by stealing the pathname associated with the
socket it is listening on).
We cannot directly use a .lock lockfile to create the socket because
the socket is created by `bind(2)` rather than the `open(2)` mechanism
used by `tempfile.c`.
As an alternative, we hold a plain lockfile ("<path>.lock") as a
mutual exclusion device. Under the lock, we test if an existing
socket ("<path>") is has an active server. If not, create a new
socket and begin listening. Then we rollback the lockfile in all
cases.
Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
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> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Create t0052-simple-ipc.sh with unit tests for the "simple-ipc" mechanism. Create t/helper/test-simple-ipc test tool to exercise the "simple-ipc" functions. When the tool is invoked with "run-daemon", it runs a server to listen for "simple-ipc" connections on a test socket or named pipe and responds to a set of commands to exercise/stress the communication setup. When the tool is invoked with "start-daemon", it spawns a "run-daemon" command in the background and waits for the server to become ready before exiting. (This helps make unit tests in t0052 more predictable and avoids the need for arbitrary sleeps in the test script.) The tool also has a series of client "send" commands to send commands and data to a server instance. Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Remove the scratch buffer argument from `write_packetized_from_fd_no_flush()` and allocate a temporary buffer within the function. In 3e4447f (pkt-line: eliminate the need for static buffer in packet_write_gently(), 2021-02-13) we added the argument in order to get rid of a static buffer to make the routine safe in multi-threaded contexts and to avoid putting a very large buffer on the stack. This delegated the problem to the caller who has more knowledge of the use case and can best decide the most efficient way to provide such a buffer. However, in practice, the (currently, only) caller just created the buffer on the stack, so we might as well just allocate it within the function and restore the original API. Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Change the public initialization of `struct unix_stream_listen_opts` to be all zeroes. Hide the default values for the timeout and backlog values inside `unix-socket.c`. Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Move unix_stream_server__listen_with_lock() and associated routines and data types out of unix-socket.[ch] and into their own source file. We added code to safely create a Unix domain socket using a lockfile in: 9406d12 (unix-socket: create `unix_stream_server__listen_with_lock()`, 2021-02-13). This code conceptually exists at a higher-level than the core unix_stream_connect() and unix_stream_listen() routines that it consumes and should be isolated for clarity. Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Move error reporting for socket/named pipe creation out of platform specific and simple-ipc layer code and rely on returned error value and `errno`. Update t/helper/test-simple-ipc.c to print errors. Simplify testing for another server in `is_another_server_is_alive()`. Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
When checking to see if our unix domain socket was stolen, also check whether the st.st_dev and st.st_mode fields changed (in addition to the st.st_ino field). The inode by itself is not unique; it is only unique on a given device. Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Refactor command line option processing in simple-ipc helper under the main "simple-ipc" command rather than in the individual subcommands. Update "start-daemon" subcommand to pass socket pathname to background process on both platforms. Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Change the `test-tool simple-ipc send` subcommand take a string option rather than assume the last unclaimed value of argv. This makes it a little less awkward to use after the recent changes that added the `--name=<name>` option. Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
|
/submit |
|
Submitted as pull.893.git.1614889047.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
convert.c
Outdated
| @@ -883,10 +883,9 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len | |||
| if (err) | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> Remove the scratch buffer argument from `write_packetized_from_fd_no_flush()`
> and allocate a temporary buffer within the function.
>
> In 3e4447f1ea (pkt-line: eliminate the need for static buffer in
> packet_write_gently(), 2021-02-13) we added the argument in order to
> get rid of a static buffer to make the routine safe in multi-threaded
> contexts and to avoid putting a very large buffer on the stack. This
> delegated the problem to the caller who has more knowledge of the use
> case and can best decide the most efficient way to provide such a
> buffer.
>
> However, in practice, the (currently, only) caller just created the
> buffer on the stack, so we might as well just allocate it within the
> function and restore the original API.
Hmph, I would have expected, since we already have changed the
callchain to pass the buffer down, that we'd keep the structure and
update the caller to heap-allocate, but I think I like the result of
this patch better. It's not like the caller can allocate and reuse
a single buffer with repeated calls to the function; rather, the
caller makes a single call that results in relaying many bytes by
the helper, all done in a loop, and it is sufficient for the helper
to allocate/deallocate outside of its loop to reuse the buffer.
unix-socket.c
Outdated
| @@ -2,6 +2,9 @@ | |||
| #include "lockfile.h" | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
> struct lock_file lock = LOCK_INIT;
> + long timeout;
> int fd_socket;
> struct unix_stream_server_socket *server_socket;
>
> + timeout = opts->timeout_ms;
> + if (opts->timeout_ms <= 0)
> + timeout = DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT;
If we have 0 as a special value to tell this API to use the default
value, do we need to treat negative values the same way?
Do we see any value in being able to say "no timeout---if we can do
so immediately fine, otherwise return me a failure"? Deep in the
callchain, lockfile.c::lock_file_timeout(), which is the workhorse
of the feature, notices timeout_ms==0 and makes a direct call to
lock_file() after all, so we are prepared for such a caller already.
And if this is such a caller that may benefit from being able to say
"fail if we cannot immediately lock", perhaps we might want to allow
0 to be used as a real value and use something else as a signal to
use the timeout value determined by the helper as the default.
IOW, I would find the above iffy and prefer any of the following
over it:
(0) if (!opts->timeout_ms)
timeout = DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT;
else if (opts->timeout_ms < 0)
BUG("...");
(1) if (opts->timeout_ms < 0)
timeout = DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT;
(2) if (opts->timeout_ms == -1)
timeout = DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT;
else if (opts->timeout_ms < 0)
BUG("...");
> diff --git a/unix-socket.h b/unix-socket.h
> index 8faf5b692f90..bec925ee0213 100644
> --- a/unix-socket.h
> +++ b/unix-socket.h
> @@ -7,13 +7,10 @@ struct unix_stream_listen_opts {
> unsigned int disallow_chdir:1;
> };
>
> -#define DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT (100)
> -#define DEFAULT_UNIX_STREAM_LISTEN_BACKLOG (5)
> -
> #define UNIX_STREAM_LISTEN_OPTS_INIT \
> { \
> - .timeout_ms = DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT, \
> - .listen_backlog_size = DEFAULT_UNIX_STREAM_LISTEN_BACKLOG, \
> + .timeout_ms = 0, \
> + .listen_backlog_size = 0, \
> .disallow_chdir = 0, \
> }
I thought the point of suggested fix was to allow 0-initialize the
whole structure, so that we do not have to have the C preprocessor
macro UNIX_STREAM_LISTEN_OPTS_INIT at all. I.e. it would allow us
to do
- struct unix_stream_listen_opts opts = UNIX_STREAM_LISTEN_OPTS_INIT;
+ struct unix_stream_listen_opts opts = { 0 };
in builtin/credential-cache--daemon.c::serve_cache().
If we cannot use 0 as a special value, however, for the timeout,
then we cannot get rid of UNIX_STREAM_LISTEN_OPTS_INIT, but at least
we should be able to do
#define UNIX_STREAM_LISTEN_OPTS_INIT { .timeout_ms = -1 }
and leave everything else 0-initialized.
Thanks.
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
|
This patch series was integrated into seen via git@bec1f00. |
|
On the Git mailing list, Jeff Hostetler wrote (reply to this): |
|
User |
|
This patch series was integrated into seen via git@db47d10. |
|
This patch series was integrated into seen via git@2db1125. |
|
This patch series was integrated into seen via git@dfe8e1a. |
|
This patch series was integrated into seen via git@99dcc12. |
|
This patch series was integrated into seen via git@1a63040. |
|
This patch series was integrated into seen via git@6bae851. |
|
This patch series was integrated into seen via git@fecbd01. |
|
This patch series was integrated into seen via git@b24af91. |
|
This patch series was integrated into seen via git@667a6e3. |
|
This patch series was integrated into seen via git@0e46061. |
|
This patch series was integrated into seen via git@1f6aaaa. |
|
This patch series was integrated into seen via git@48491b7. |
|
This patch series was integrated into seen via git@8652114. |
|
This patch series was integrated into seen via git@d562ddd. |
|
This patch series was integrated into seen via git@17271fa. |
|
This patch series was integrated into seen via git@3ecd81b. |
|
This patch series was integrated into seen via git@32f3cf4. |
|
This patch series was integrated into next via git@3ebcedf. |
|
This patch series was integrated into seen via git@bc9ebc4. |
|
This patch series was integrated into seen via git@910d4f2. |
|
This patch series was integrated into seen via git@99e734d. |
|
This patch series was integrated into seen via git@ff338b5. |
|
This patch series was integrated into seen via git@861794b. |
|
This patch series was integrated into next via git@861794b. |
|
This patch series was integrated into master via git@861794b. |
|
Closed via 861794b. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch series adds a few final cleanup and refactoring commits onto
the
jh/simple-ipcbranch. These are in response to mailing list commentsthat I received on my V4 version after it was queued into
next.https://lore.kernel.org/git/pull.766.v4.git.1613598529.gitgitgadget@gmail.com/T/#mbd1da5ff93ef273049090f697aeab68c74f698f1
There are a couple of large changes that I'll call out here.
In the third commit, I moved the new lock-aware code out of unix-socket.c and
into its own source file. This creates a slightly misleading diff at times (in
gitk)where it looks like 51% copy of unix-socket.c rather than a new file. On the command
line and on GitHub it looks better.
In the commits prefixed with
test-simple-ipc: ...I refactored the options parsingto allow the name of the socket/named-pipe to be passed on the command line
so that the Windows version could do so (since it needs to exec a child rather than
fork). This turned into a larger cleanup/refactoring than I had expected, but I think
the result is much better. I unified all of the option parsing into the main
cmd__simple_ipc()function and got rid of the smaller parsers inside of eachsubcommand. With this, all of the subcommands now allow an alternate socket
path to be used. (Just fixing the unused arg on the Windows side would allow us
to spawn a background daemon on a different socket, but none of the client
subcommands would be able to talk to it.)
cc: Jeff Hostetler git@jeffhostetler.com
cc: René Scharfe l.s.r@web.de
cc: Jeff Hostetler git@jeffhostetler.com