From bb01122a82002dc5948147800da40a149779a7b1 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 24 Aug 2021 15:43:59 +0000 Subject: maintenance: create `launchctl` configuration using a lock file When two `git maintenance` processes try to write the `.plist` file, we need to help them with serializing their efforts. The 150ms time-out value was determined from thin air. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- builtin/gc.c | 47 ++++++++++++++++++++++++++++++----------------- 1 file changed, 30 insertions(+), 17 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index f05d2f0a1a..06f18a347d 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1602,16 +1602,14 @@ static int launchctl_remove_plists(const char *cmd) static int launchctl_schedule_plist(const char *exec_path, enum schedule_priority schedule, const char *cmd) { - FILE *plist; - int i; + int i, fd; const char *preamble, *repeat; const char *frequency = get_frequency(schedule); char *name = launchctl_service_name(frequency); char *filename = launchctl_service_filename(name); - - if (safe_create_leading_directories(filename)) - die(_("failed to create directories for '%s'"), filename); - plist = xfopen(filename, "w"); + struct lock_file lk = LOCK_INIT; + static unsigned long lock_file_timeout_ms = ULONG_MAX; + struct strbuf plist = STRBUF_INIT; preamble = "\n" "\n" @@ -1630,7 +1628,7 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit "\n" "StartCalendarInterval\n" "\n"; - fprintf(plist, preamble, name, exec_path, exec_path, frequency); + strbuf_addf(&plist, preamble, name, exec_path, exec_path, frequency); switch (schedule) { case SCHEDULE_HOURLY: @@ -1639,7 +1637,7 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit "Minute0\n" "\n"; for (i = 1; i <= 23; i++) - fprintf(plist, repeat, i); + strbuf_addf(&plist, repeat, i); break; case SCHEDULE_DAILY: @@ -1649,24 +1647,38 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit "Minute0\n" "\n"; for (i = 1; i <= 6; i++) - fprintf(plist, repeat, i); + strbuf_addf(&plist, repeat, i); break; case SCHEDULE_WEEKLY: - fprintf(plist, - "\n" - "Day0\n" - "Hour0\n" - "Minute0\n" - "\n"); + strbuf_addstr(&plist, + "\n" + "Day0\n" + "Hour0\n" + "Minute0\n" + "\n"); break; default: /* unreachable */ break; } - fprintf(plist, "\n\n\n"); - fclose(plist); + strbuf_addstr(&plist, "\n\n\n"); + + if (safe_create_leading_directories(filename)) + die(_("failed to create directories for '%s'"), filename); + + if ((long)lock_file_timeout_ms < 0 && + git_config_get_ulong("gc.launchctlplistlocktimeoutms", + &lock_file_timeout_ms)) + lock_file_timeout_ms = 150; + + fd = hold_lock_file_for_update_timeout(&lk, filename, LOCK_DIE_ON_ERROR, + lock_file_timeout_ms); + + if (write_in_full(fd, plist.buf, plist.len) < 0 || + commit_lock_file(&lk)) + die_errno(_("could not write '%s'"), filename); /* bootout might fail if not already running, so ignore */ launchctl_boot_plist(0, filename, cmd); @@ -1675,6 +1687,7 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit free(filename); free(name); + strbuf_release(&plist); return 0; } -- cgit v1.2.3 From a16eb6b1ff333f3e26b99f17ef3bb7dbf8135f39 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Tue, 24 Aug 2021 15:44:00 +0000 Subject: maintenance: skip bootout/bootstrap when plist is registered On macOS, we use launchctl to manage the background maintenance schedule. This uses a set of .plist files to describe the schedule, but these files are also registered with 'launchctl bootstrap'. If multiple 'git maintenance start' commands run concurrently, then they can collide replacing these schedule files and registering them with launchctl. To avoid extra launchctl commands, do a check for the .plist files on disk and check if they are registered using 'launchctl list '. This command will return with exit code 0 if it exists, or exit code 113 if it does not. We can test this behavior using the GIT_TEST_MAINT_SCHEDULER environment variable. Signed-off-by: Derrick Stolee Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- builtin/gc.c | 54 +++++++++++++++++++++++++++++++++++++++++--------- t/t7900-maintenance.sh | 17 ++++++++++++++++ 2 files changed, 62 insertions(+), 9 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index 06f18a347d..22e670b508 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1600,6 +1600,29 @@ static int launchctl_remove_plists(const char *cmd) launchctl_remove_plist(SCHEDULE_WEEKLY, cmd); } +static int launchctl_list_contains_plist(const char *name, const char *cmd) +{ + int result; + struct child_process child = CHILD_PROCESS_INIT; + char *uid = launchctl_get_uid(); + + strvec_split(&child.args, cmd); + strvec_pushl(&child.args, "list", name, NULL); + + child.no_stderr = 1; + child.no_stdout = 1; + + if (start_command(&child)) + die(_("failed to start launchctl")); + + result = finish_command(&child); + + free(uid); + + /* Returns failure if 'name' doesn't exist. */ + return !result; +} + static int launchctl_schedule_plist(const char *exec_path, enum schedule_priority schedule, const char *cmd) { int i, fd; @@ -1609,7 +1632,8 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit char *filename = launchctl_service_filename(name); struct lock_file lk = LOCK_INIT; static unsigned long lock_file_timeout_ms = ULONG_MAX; - struct strbuf plist = STRBUF_INIT; + struct strbuf plist = STRBUF_INIT, plist2 = STRBUF_INIT; + struct stat st; preamble = "\n" "\n" @@ -1676,18 +1700,30 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit fd = hold_lock_file_for_update_timeout(&lk, filename, LOCK_DIE_ON_ERROR, lock_file_timeout_ms); - if (write_in_full(fd, plist.buf, plist.len) < 0 || - commit_lock_file(&lk)) - die_errno(_("could not write '%s'"), filename); - - /* bootout might fail if not already running, so ignore */ - launchctl_boot_plist(0, filename, cmd); - if (launchctl_boot_plist(1, filename, cmd)) - die(_("failed to bootstrap service %s"), filename); + /* + * Does this file already exist? With the intended contents? Is it + * registered already? Then it does not need to be re-registered. + */ + if (!stat(filename, &st) && st.st_size == plist.len && + strbuf_read_file(&plist2, filename, plist.len) == plist.len && + !strbuf_cmp(&plist, &plist2) && + launchctl_list_contains_plist(name, cmd)) + rollback_lock_file(&lk); + else { + if (write_in_full(fd, plist.buf, plist.len) < 0 || + commit_lock_file(&lk)) + die_errno(_("could not write '%s'"), filename); + + /* bootout might fail if not already running, so ignore */ + launchctl_boot_plist(0, filename, cmd); + if (launchctl_boot_plist(1, filename, cmd)) + die(_("failed to bootstrap service %s"), filename); + } free(filename); free(name); strbuf_release(&plist); + strbuf_release(&plist2); return 0; } diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh index 58f46c77e6..fc16ac2258 100755 --- a/t/t7900-maintenance.sh +++ b/t/t7900-maintenance.sh @@ -582,6 +582,23 @@ test_expect_success 'start and stop macOS maintenance' ' test_line_count = 0 actual ' +test_expect_success 'use launchctl list to prevent extra work' ' + # ensure we are registered + GIT_TEST_MAINT_SCHEDULER=launchctl:./print-args git maintenance start && + + # do it again on a fresh args file + rm -f args && + GIT_TEST_MAINT_SCHEDULER=launchctl:./print-args git maintenance start && + + ls "$HOME/Library/LaunchAgents" >actual && + cat >expect <<-\EOF && + list org.git-scm.git.hourly + list org.git-scm.git.daily + list org.git-scm.git.weekly + EOF + test_cmp expect args +' + test_expect_success 'start and stop Windows maintenance' ' write_script print-args <<-\EOF && echo $* >>args -- cgit v1.2.3