| Message ID | pull.766.v4.git.1613598529.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Simple IPC Mechanism | expand |
"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes: > Here is V4 of my "Simple IPC" series. It addresses Gábor's comment WRT > shutting down the server to make unit tests more predictable on CI servers. > (https://lore.kernel.org/git/20210213093052.GJ1015009@szeder.dev) > > Jeff > > cc: Ævar Arnfjörð Bjarmason avarab@gmail.com cc: Jeff Hostetler > git@jeffhostetler.com cc: Jeff King peff@peff.net cc: Chris Torek > chris.torek@gmail.com It seems that the discussions around the topic has mostly done during the v2 review, and has quieted down since then. Let's merge it down to 'next'?
On Thu, Feb 25, 2021 at 11:39:39AM -0800, Junio C Hamano wrote: > "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > Here is V4 of my "Simple IPC" series. It addresses Gábor's comment WRT > > shutting down the server to make unit tests more predictable on CI servers. > > (https://lore.kernel.org/git/20210213093052.GJ1015009@szeder.dev) > > > > Jeff > > > > cc: Ævar Arnfjörð Bjarmason avarab@gmail.com cc: Jeff Hostetler > > git@jeffhostetler.com cc: Jeff King peff@peff.net cc: Chris Torek > > chris.torek@gmail.com > > It seems that the discussions around the topic has mostly done > during the v2 review, and has quieted down since then. > > Let's merge it down to 'next'? Sorry, I hadn't gotten around to looking at the latest version. I left another round of comments. Some of them are arguably bikeshedding, but there's at least one I think we'd want to address (the big stack buffer in patch 1). I also haven't carefully looked at the simple-ipc design at all; my focus has just been on the details of socket and pktline code being touched. Since there are no simple-ipc users yet, and since it's internal and would be easy to change later, I'm mostly content for Jeff to proceed as he sees fit and iterate on it as necessary. -Peff
On 2/26/21 2:59 AM, Jeff King wrote: > On Thu, Feb 25, 2021 at 11:39:39AM -0800, Junio C Hamano wrote: > >> "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes: >> >>> Here is V4 of my "Simple IPC" series. It addresses Gábor's comment WRT >>> shutting down the server to make unit tests more predictable on CI servers. >>> (https://lore.kernel.org/git/20210213093052.GJ1015009@szeder.dev) >>> >>> Jeff >>> >>> cc: Ævar Arnfjörð Bjarmason avarab@gmail.com cc: Jeff Hostetler >>> git@jeffhostetler.com cc: Jeff King peff@peff.net cc: Chris Torek >>> chris.torek@gmail.com >> >> It seems that the discussions around the topic has mostly done >> during the v2 review, and has quieted down since then. >> >> Let's merge it down to 'next'? > > Sorry, I hadn't gotten around to looking at the latest version. I left > another round of comments. Some of them are arguably bikeshedding, but > there's at least one I think we'd want to address (the big stack buffer > in patch 1). > > I also haven't carefully looked at the simple-ipc design at all; my > focus has just been on the details of socket and pktline code being > touched. Since there are no simple-ipc users yet, and since it's > internal and would be easy to change later, I'm mostly content for Jeff > to proceed as he sees fit and iterate on it as necessary. > > -Peff > We can wait until next week on moving this 'next' if you want. I'll attend to the buffer alloc in patch 1. I'm still reading the other comments and will see where that takes me. I'm about ready to push an RFC for my fsmonitor--daemon series that sits on top of this simple-ipc series, so you can see an actual use case if that would help understand (my madness). Thanks Jeff
On Fri, Feb 26, 2021 at 03:18:26PM -0500, Jeff Hostetler wrote: > > Sorry, I hadn't gotten around to looking at the latest version. I left > > another round of comments. Some of them are arguably bikeshedding, but > > there's at least one I think we'd want to address (the big stack buffer > > in patch 1). > > > > I also haven't carefully looked at the simple-ipc design at all; my > > focus has just been on the details of socket and pktline code being > > touched. Since there are no simple-ipc users yet, and since it's > > internal and would be easy to change later, I'm mostly content for Jeff > > to proceed as he sees fit and iterate on it as necessary. > > We can wait until next week on moving this 'next' if you want. > I'll attend to the buffer alloc in patch 1. I'm still reading the > other comments and will see where that takes me. I could have been a bit more clear here: modulo any response you have to my latest round of comments, I'm mostly happy to let this proceed to next. So I was thinking you'd have one more re-roll dealing with the patch 1 problems plus anything else you think worth addressing from my batch of comments, and then that result would probably be ready for 'next'. > I'm about ready to push an RFC for my fsmonitor--daemon series that > sits on top of this simple-ipc series, so you can see an actual use > case if that would help understand (my madness). I may have dug my own grave here. ;) I'm actually not incredibly interested in the overall topic. So I wasn't saying so much "I'll reserve judgement on simple-ipc until I see callers" so much as "I expect you'll find any shortcomings in its design yourself as you build on top of it". And by "not interested" I don't mean that I think the topic is without value. Far from it; I think this is an important area to be working in. But it's complex and time-consuming to review. So I was hoping somebody with more expertise and interest in the problem space would do that part of the review, and I could continue to focus on other stuff. That may be wishful thinking, though. :) -Peff
Jeff King <peff@peff.net> writes: > And by "not interested" I don't mean that I think the topic is without > value. Far from it; I think this is an important area to be working in. > But it's complex and time-consuming to review. So I was hoping somebody > with more expertise and interest in the problem space would do that part > of the review, and I could continue to focus on other stuff. That may be > wishful thinking, though. :) I was not paying close attention to this series, and was planning to visit it before merging it to 'next' but only to ensure that changes to any existing code would not regress existing callers, so it seems that we two have been with pretty much the same attitude;-)
Here is V4 of my "Simple IPC" series. It addresses Gábor's comment WRT shutting down the server to make unit tests more predictable on CI servers. (https://lore.kernel.org/git/20210213093052.GJ1015009@szeder.dev) Jeff cc: Ævar Arnfjörð Bjarmason avarab@gmail.com cc: Jeff Hostetler git@jeffhostetler.com cc: Jeff King peff@peff.net cc: Chris Torek chris.torek@gmail.com 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: elimiate 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-socket: create `unix_stream_server__listen_with_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: (optionally) libify the packet readers pkt-line: add options argument to read_packetized_to_strbuf() Documentation/technical/api-simple-ipc.txt | 34 + Makefile | 8 + 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 | 979 +++++++++++++++++++++ compat/simple-ipc/ipc-win32.c | 749 ++++++++++++++++ config.mak.uname | 2 + contrib/buildsystems/CMakeLists.txt | 6 + convert.c | 16 +- pkt-line.c | 57 +- pkt-line.h | 20 +- simple-ipc.h | 235 +++++ t/helper/test-simple-ipc.c | 773 ++++++++++++++++ t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + t/t0052-simple-ipc.sh | 122 +++ unix-socket.c | 168 +++- unix-socket.h | 47 +- 19 files changed, 3198 insertions(+), 53 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: 773e25afc41b1b6533fa9ae2cd825d0b4a697fad Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-766%2Fjeffhostetler%2Fsimple-ipc-v4 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-766/jeffhostetler/simple-ipc-v4 Pull-Request: https://github.com/gitgitgadget/git/pull/766 Range-diff vs v3: 1: 2d6858b1625a = 1: 2d6858b1625a pkt-line: eliminate the need for static buffer in packet_write_gently() 2: 91a9f63d6692 = 2: 91a9f63d6692 pkt-line: do not issue flush packets in write_packetized_*() 3: e05467def4e1 = 3: e05467def4e1 pkt-line: (optionally) libify the packet readers 4: 81e14bed955c = 4: 81e14bed955c pkt-line: add options argument to read_packetized_to_strbuf() 5: 22eec60761a8 = 5: 22eec60761a8 simple-ipc: design documentation for new IPC mechanism 6: 171ec43ecfa4 = 6: 171ec43ecfa4 simple-ipc: add win32 implementation 7: b368318e6a23 = 7: b368318e6a23 unix-socket: elimiate static unix_stream_socket() helper function 8: 985b2e02b2df = 8: 985b2e02b2df unix-socket: add backlog size option to unix_stream_listen() 9: 1bfa36409d07 = 9: 1bfa36409d07 unix-socket: disallow chdir() when creating unix domain sockets 10: b443e11ac32f = 10: b443e11ac32f unix-socket: create `unix_stream_server__listen_with_lock()` 11: 43c8db9a4468 = 11: 43c8db9a4468 simple-ipc: add Unix domain socket implementation 12: 1e5c856ade85 ! 12: 09568a6500dd t0052: add simple-ipc tests and t/helper/test-simple-ipc tool @@ t/helper/test-simple-ipc.c (new) +static ipc_server_application_cb test_app_cb; + +/* -+ * This is "application callback" that sits on top of the "ipc-server". -+ * It completely defines the set of command verbs supported by this -+ * application. ++ * This is the "application callback" that sits on top of the ++ * "ipc-server". It completely defines the set of commands supported ++ * by this application. + */ +static int test_app_cb(void *application_data, + const char *command, @@ t/helper/test-simple-ipc.c (new) + * Send an IPC command to an already-running server daemon and print the + * response. + * -+ * argv[2] contains a simple (1 word) command verb that `test_app_cb()` -+ * (in the daemon process) will understand. ++ * argv[2] contains a simple (1 word) command that `test_app_cb()` (in ++ * the daemon process) will understand. + */ +static int client__send_ipc(int argc, const char **argv, const char *path) +{ @@ t/helper/test-simple-ipc.c (new) +} + +/* ++ * Send an IPC command to an already-running server and ask it to ++ * shutdown. "send quit" is an async request and queues a shutdown ++ * event in the server, so we spin and wait here for it to actually ++ * shutdown to make the unit tests a little easier to write. ++ */ ++static int client__stop_server(int argc, const char **argv, const char *path) ++{ ++ const char *send_quit[] = { argv[0], "send", "quit", NULL }; ++ int max_wait_sec = 60; ++ int ret; ++ time_t time_limit, now; ++ enum ipc_active_state s; ++ ++ const char * const stop_usage[] = { ++ N_("test-helper simple-ipc stop-daemon [<options>]"), ++ NULL ++ }; ++ ++ struct option stop_options[] = { ++ OPT_INTEGER(0, "max-wait", &max_wait_sec, ++ N_("seconds to wait for daemon to stop")), ++ OPT_END() ++ }; ++ ++ argc = parse_options(argc, argv, NULL, stop_options, stop_usage, 0); ++ ++ if (max_wait_sec < 0) ++ max_wait_sec = 0; ++ ++ time(&time_limit); ++ time_limit += max_wait_sec; ++ ++ ret = client__send_ipc(3, send_quit, path); ++ if (ret) ++ return ret; ++ ++ for (;;) { ++ sleep_millisec(100); ++ ++ s = ipc_get_active_state(path); ++ ++ if (s != IPC_STATE__LISTENING) { ++ /* ++ * The socket/pipe is gone and/or has stopped ++ * responding. Lets assume that the daemon ++ * process has exited too. ++ */ ++ return 0; ++ } ++ ++ time(&now); ++ if (now > time_limit) ++ return error(_("daemon has not shutdown yet")); ++ } ++} ++ ++/* + * Send an IPC command followed by ballast to confirm that a large + * message can be sent and that the kernel or pkt-line layers will + * properly chunk it and that the daemon receives the entire message. @@ t/helper/test-simple-ipc.c (new) + if (client__probe_server(path)) + return 1; + ++ if (argc >= 2 && !strcmp(argv[1], "stop-daemon")) ++ return !!client__stop_server(argc, argv, path); ++ + if ((argc == 2 || argc == 3) && !strcmp(argv[1], "send")) + return !!client__send_ipc(argc, argv, path); + @@ t/t0052-simple-ipc.sh (new) +} + +stop_simple_IPC_server () { -+ test-tool simple-ipc send quit ++ test-tool simple-ipc stop-daemon +} + +test_expect_success 'start simple command server' ' @@ t/t0052-simple-ipc.sh (new) + test_cmp expect_a actual_a +' + -+# Sending a "quit" message to the server causes it to start an "async -+# shutdown" -- queuing shutdown events to all socket/pipe thread-pool -+# threads. Each thread will process that event after finishing -+# (draining) any in-progress IO with other clients. So when the "send -+# quit" client command exits, the ipc-server may still be running (but -+# it should be cleaning up). -+# -+# So, insert a generous sleep here to give the server time to shutdown. -+# -+test_expect_success '`quit` works' ' -+ test-tool simple-ipc send quit && -+ -+ sleep 5 && -+ ++test_expect_success 'stop-daemon works' ' ++ test-tool simple-ipc stop-daemon && + test_must_fail test-tool simple-ipc is-active && + test_must_fail test-tool simple-ipc send ping +'