fsmonitor OSX: fix hangs for submodules

PATCH Superseded Koji Nakamaru via GitGitGadget 4 participants 19 messages Sep 29, 2024 – Oct 5, 2024

lore

Share & tools

lore

Koji Nakamaru via GitGitGadget

author

2024-09-29 02:41 UTC #0 lore raw

PATCH 1/1 fsmonitor OSX: fix hangs for submodules

From: Koji Nakamaru <[email protected]>

fsmonitor_classify_path_absolute() expects state->path_gitdir_watch.buf has no trailing '/' or '.' For a submodule, fsmonitor_run_daemon() sets the value with trailing "/." (as repo_get_git_dir(the_repository) on Darwin returns ".") so that fsmonitor_classify_path_absolute() returns IS_OUTSIDE_CONE.

In this case, fsevent_callback() doesn't update cookie_list so that fsmonitor_publish() does nothing and with_lock__mark_cookies_seen() is not invoked.

As with_lock__wait_for_cookie() infinitely waits for state->cookies_cond that with_lock__mark_cookies_seen() should unlock, the whole daemon hangs.

Remove trailing "/." from state->path_gitdir_watch.buf for submodules and add a corresponding test in t7527-builtin-fsmonitor.sh.

Helped-by Johannes Schindelin <[email protected]>

Signed-off-by Koji Nakamaru <[email protected]>

---
fsmonitor/darwin: fix hangs for submodules

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1802%2FKojiNakamaru%2Ffix%2Ffsmonitor-darwin-hangs-for-submodules-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1802/KojiNakamaru/fix/fsmonitor-darwin-hangs-for-submodules-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1802

 builtin/fsmonitor--daemon.c  |  1 +
 t/t7527-builtin-fsmonitor.sh | 39 ++++++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

builtin/fsmonitor--daemon.c +1 -0

index dce8a3b2482..e1e6b96d09e 100644
@@ -1314,6 +1314,7 @@ static int fsmonitor_run_daemon(void)
 		strbuf_reset(&state.path_gitdir_watch);
 		strbuf_addstr(&state.path_gitdir_watch,
 			      absolute_path(repo_get_git_dir(the_repository)));
+		strbuf_strip_suffix(&state.path_gitdir_watch, "/.");
 		state.nr_paths_watching = 2;
 	}

t/t7527-builtin-fsmonitor.sh +39 -0

index 730f3c7f810..7acd074a97f 100755
@@ -82,6 +82,28 @@ have_t2_data_event () {
 	grep -e '"event":"data".*"category":"'"$c"'".*"key":"'"$k"'"'
 }
 
+start_git_in_background () {
+	git "$@" &
+	git_pid=$!
+	nr_tries_left=10
+	while true
+	do
+		if test $nr_tries_left -eq 0
+		then
+			kill $git_pid
+			exit 1
+		fi
+		sleep 1
+		nr_tries_left=$(($nr_tries_left - 1))
+	done > /dev/null 2>&1 &
+	watchdog_pid=$!
+	wait $git_pid
+}
+
+stop_git_and_watchdog () {
+	kill $git_pid $watchdog_pid
+}
+
 test_expect_success 'explicit daemon start and stop' '
 	test_when_finished "stop_daemon_delete_repo test_explicit" &&

@@ -907,6 +929,23 @@ test_expect_success "submodule absorbgitdirs implicitly starts daemon" '
 	test_subcommand git fsmonitor--daemon start <super-sub.trace
 '
 
+test_expect_success "submodule implicitly starts daemon by pull" '
+	test_atexit "stop_git_and_watchdog" &&
+	test_when_finished "rm -rf cloned; \
+			    rm -rf super; \
+			    rm -rf sub" &&
+
+	create_super super &&
+	create_sub sub &&
+
+	git -C super submodule add ../sub ./dir_1/dir_2/sub &&
+	git -C super commit -m "add sub" &&
+	git clone --recurse-submodules super cloned &&
+
+	git -C cloned/dir_1/dir_2/sub config core.fsmonitor true &&
+	start_git_in_background -C cloned pull --recurse-submodules
+'
+
 # On a case-insensitive file system, confirm that the daemon
 # notices when the .git directory is moved/renamed/deleted
 # regardless of how it is spelled in the FS event.

base-commit: 3857aae53f3633b7de63ad640737c657387ae0c6

Signature · 1 line

gitgitgadget

Junio C Hamano

2024-09-30 17:40 UTC #1 lore raw

Quoted 8 lines · Koji Nakamaru via GitGitGadget

From: Koji Nakamaru <[email protected]>
fsmonitor_classify_path_absolute() expects state->path_gitdir_watch.buf has no trailing '/' or '.' For a submodule, fsmonitor_run_daemon() sets the value with trailing "/." (as repo_get_git_dir(the_repository) on Darwin returns ".") so that fsmonitor_classify_path_absolute() returns IS_OUTSIDE_CONE.

Quoted 7 lines

In this case, fsevent_callback() doesn't update cookie_list so that fsmonitor_publish() does nothing and with_lock__mark_cookies_seen() is not invoked.
As with_lock__wait_for_cookie() infinitely waits for state->cookies_cond that with_lock__mark_cookies_seen() should unlock, the whole daemon hangs.

The above very nicely describes the cause, the mechansim that leads to the end-user observable effect, and the (bad) effect the bug has.

I wish everybody wrote their proposed commit messages like this ;-)

Quoted 20 lines

Remove trailing "/." from state->path_gitdir_watch.buf for submodules and add a corresponding test in t7527-builtin-fsmonitor.sh.
Helped-by Johannes Schindelin <[email protected]>
Signed-off-by Koji Nakamaru <[email protected]>
---
fsmonitor/darwin: fix hangs for submodules
t/t7527-builtin-fsmonitor.sh +4 -0
index 730f3c7f810..7acd074a97f 100755
@@ -82,6 +82,28 @@ have_t2_data_event () {
 	grep -e '"event":"data".*"category":"'"$c"'".*"key":"'"$k"'"'
 }
 
+start_git_in_background () {
+	git "$@" &
+	git_pid=$!
+	nr_tries_left=10

t/t7527-builtin-fsmonitor.sh have_t2_data_event () { · line 98

Quoted 10 lines

t/t7527-builtin-fsmonitor.sh +10 -0

+	while true
+	do
+		if test $nr_tries_left -eq 0
+		then
+			kill $git_pid
+			exit 1
+		fi
+		sleep 1
+		nr_tries_left=$(($nr_tries_left - 1))
+	done > /dev/null 2>&1 &

So, the command is allowed to run for 10 seconds and then a signal is sent to the process (by the way, we do not write the SP between ">" and "/dev/null").

t/t7527-builtin-fsmonitor.sh have_t2_data_event () { · line 100

Quoted 2 lines

t/t7527-builtin-fsmonitor.sh +2 -0
+	watchdog_pid=$!
+	wait $git_pid

And the process to ensure the command gets killed in 10 seconds is called the "watchdog". We let the command run for completion (and we'd be happy if it did without watchdog needing to forcibly kill it).

Which means that even after the test finishes normally (e.g., the command completes without getting killed by the watchdog, because it is on a fast box and finishes in 0.5 second), we have leftover watchdog process hanging around for 10 seconds, which might interfere with the removal of the $TRASH_DIRECTORY at the end of the test.

There is a helper function to kill both (below), which probably is used to avoid it. Let's keep reading.

t/t7527-builtin-fsmonitor.sh have_t2_data_event () { · line 105

Quoted 5 lines

t/t7527-builtin-fsmonitor.sh +5 -0

+}
+
+stop_git_and_watchdog () {
+	kill $git_pid $watchdog_pid
+}

This sends a signal and let the process die. Without waiting to make sure they indeed died, at which point we can safely remove the $TRASH_DIRECTORY on filesystems that refuse to remove a directory when a process still has it as its current working directory.

Shouldn't it loop, like

for pid in $git_pid $watchdog_pid
do
until kill -0 $pid
do
kill $pid
done
done

or something? Or is there a mechanism already to ensure that we return after they get killed that I am failing to find?

t/t7527-builtin-fsmonitor.sh test_expect_success "submodule absorbgitdirs implicitly starts daemon" ' · line 933

Quoted 9 lines

t/t7527-builtin-fsmonitor.sh +2 -0

 test_expect_success 'explicit daemon start and stop' '
 	test_when_finished "stop_daemon_delete_repo test_explicit" &&

@@ -907,6 +929,23 @@ test_expect_success "submodule absorbgitdirs implicitly starts daemon" '
 	test_subcommand git fsmonitor--daemon start <super-sub.trace
 '
 
+test_expect_success "submodule implicitly starts daemon by pull" '
+	test_atexit "stop_git_and_watchdog" &&

Hmph, this is _atexit and not _when_finished because...?

t/t7527-builtin-fsmonitor.sh test_expect_success "submodule absorbgitdirs implicitly starts daemon" ' · line 936

Quoted 3 lines

t/t7527-builtin-fsmonitor.sh +3 -0

+	test_when_finished "rm -rf cloned; \
+			    rm -rf super; \
+			    rm -rf sub" &&

Makes me wonder why it is not written like so:

	test_when_finished "rm -rf cloned super sub" &&

which is short enough to still fit on a line. Is there something I am missing that these directories must be removed separately and in this order?

t/t7527-builtin-fsmonitor.sh test_expect_success "submodule absorbgitdirs implicitly starts daemon" ' · line 947

Quoted 10 lines

t/t7527-builtin-fsmonitor.sh +10 -0

+	create_super super &&
+	create_sub sub &&
+
+	git -C super submodule add ../sub ./dir_1/dir_2/sub &&
+	git -C super commit -m "add sub" &&
+	git clone --recurse-submodules super cloned &&
+
+	git -C cloned/dir_1/dir_2/sub config core.fsmonitor true &&
+	start_git_in_background -C cloned pull --recurse-submodules
+'

Other than that, very nicely done.

Thanks.

Quoted 5 lines

t/t7527-builtin-fsmonitor.sh +0 -0
 # On a case-insensitive file system, confirm that the daemon
 # notices when the .git directory is moved/renamed/deleted
 # regardless of how it is spelled in the FS event.
base-commit: 3857aae53f3633b7de63ad640737c657387ae0c6

Koji Nakamaru

2024-10-01 04:12 UTC #2 lore raw

Thank you very much for carefully checking the patch and suggesting better ways. I'll later revise it and submit a new one.

Quoted 21 lines

t/t7527-builtin-fsmonitor.sh have_t2_data_event () { · line 105
Quoted 5 lines
t/t7527-builtin-fsmonitor.sh +5 -0
+}
+
+stop_git_and_watchdog () {
+     kill $git_pid $watchdog_pid
+}
This sends a signal and let the process die. Without waiting to make sure they indeed died, at which point we can safely remove the $TRASH_DIRECTORY on filesystems that refuse to remove a directory when a process still has it as its current working directory.
Shouldn't it loop, like
for pid in $git_pid $watchdog_pid
do
until kill -0 $pid
do
kill $pid
done
done

Quoted 2 lines

or something? Or is there a mechanism already to ensure that we return after they get killed that I am failing to find?

I agree that we have to wait for pids. I also realized that we should run git in another process group and kill the group for killing all git child processes. I'll fix the code.

Quoted 11 lines

t/t7527-builtin-fsmonitor.sh test_expect_success "submodule absorbgitdirs implicitly starts daemon" ' · line 933
Quoted 9 lines
test_expect_success 'explicit daemon start and stop' '
      test_when_finished "stop_daemon_delete_repo test_explicit" &&
t/t7527-builtin-fsmonitor.sh +2 -0
@@ -907,6 +929,23 @@ test_expect_success "submodule absorbgitdirs implicitly starts daemon" '
      test_subcommand git fsmonitor--daemon start <super-sub.trace
 '
 
+test_expect_success "submodule implicitly starts daemon by pull" '
+     test_atexit "stop_git_and_watchdog" &&
Hmph, this is _atexit and not _when_finished because...?

This is because README describes _atexit to run unconditionally to clean up before the test script exits, e.g. to stop (kill) a daemon. More appropriately, we should kill git before "rm -rf cloned super sub" in _when_finished and kill watchdog in _atexit. I'll adjust the code.

Quoted 11 lines

t/t7527-builtin-fsmonitor.sh test_expect_success "submodule absorbgitdirs implicitly starts daemon" ' · line 936
Quoted 3 lines
t/t7527-builtin-fsmonitor.sh +3 -0
+     test_when_finished "rm -rf cloned; \
+                         rm -rf super; \
+                         rm -rf sub" &&
Makes me wonder why it is not written like so:
        test_when_finished "rm -rf cloned super sub" &&
which is short enough to still fit on a line. Is there something I am missing that these directories must be removed separately and in this order?

There is no special reason, I simply followed the style used in t7527-builtin-fsmonitor.sh. I'll fix this part.

Koji Nakamaru

Koji Nakamaru via GitGitGadget

author

2024-10-01 05:09 UTC #3 lore raw

PATCH v2 1/1 fsmonitor OSX: fix hangs for submodules

In reply to Koji Nakamaru via GitGitGadget

From: Koji Nakamaru <[email protected]>

In this case, fsevent_callback() doesn't update cookie_list so that fsmonitor_publish() does nothing and with_lock__mark_cookies_seen() is not invoked.

As with_lock__wait_for_cookie() infinitely waits for state->cookies_cond that with_lock__mark_cookies_seen() should unlock, the whole daemon hangs.

Remove trailing "/." from state->path_gitdir_watch.buf for submodules and add a corresponding test in t7527-builtin-fsmonitor.sh.

Signed-off-by Koji Nakamaru <[email protected]>

---
fsmonitor/darwin: fix hangs for submodules

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1802%2FKojiNakamaru%2Ffix%2Ffsmonitor-darwin-hangs-for-submodules-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1802/KojiNakamaru/fix/fsmonitor-darwin-hangs-for-submodules-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1802

Range-diff vs v1:

1: 1889cbb193d ! 1: decf68499f7 fsmonitor OSX: fix hangs for submodules

     @@ Commit message
          Remove trailing "/." from state->path_gitdir_watch.buf for submodules
          and add a corresponding test in t7527-builtin-fsmonitor.sh.
      
     -    Helped-by: Johannes Schindelin <[email protected]>
     +    Suggested-by: Johannes Schindelin <[email protected]>
     +    Suggested-by: Junio C Hamano <[email protected]>
          Signed-off-by: Koji Nakamaru <[email protected]>
      
       ## builtin/fsmonitor--daemon.c ##
     @@ builtin/fsmonitor--daemon.c: static int fsmonitor_run_daemon(void)
       
      
       ## t/t7527-builtin-fsmonitor.sh ##
     -@@ t/t7527-builtin-fsmonitor.sh: have_t2_data_event () {
     - 	grep -e '"event":"data".*"category":"'"$c"'".*"key":"'"$k"'"'
     - }
     +@@ t/t7527-builtin-fsmonitor.sh: test_expect_success "submodule absorbgitdirs implicitly starts daemon" '
     + 	test_subcommand git fsmonitor--daemon start <super-sub.trace
     + '
       
      +start_git_in_background () {
      +	git "$@" &
      +	git_pid=$!
     ++	git_pgid=$(ps -o pgid= -p $git_pid)
      +	nr_tries_left=10
      +	while true
      +	do
      +		if test $nr_tries_left -eq 0
      +		then
     -+			kill $git_pid
     ++			kill -- -$git_pgid
      +			exit 1
      +		fi
      +		sleep 1
      +		nr_tries_left=$(($nr_tries_left - 1))
     -+	done > /dev/null 2>&1 &
     ++	done >/dev/null 2>&1 &
      +	watchdog_pid=$!
      +	wait $git_pid
      +}
      +
     -+stop_git_and_watchdog () {
     -+	kill $git_pid $watchdog_pid
     ++stop_git () {
     ++	while kill -0 -- -$git_pgid
     ++	do
     ++		kill -- -$git_pgid
     ++		sleep 1
     ++	done
     ++}
     ++
     ++stop_watchdog () {
     ++	while kill -0 $watchdog_pid
     ++	do
     ++		kill $watchdog_pid
     ++		sleep 1
     ++	done
      +}
      +
     - test_expect_success 'explicit daemon start and stop' '
     - 	test_when_finished "stop_daemon_delete_repo test_explicit" &&
     - 
     -@@ t/t7527-builtin-fsmonitor.sh: test_expect_success "submodule absorbgitdirs implicitly starts daemon" '
     - 	test_subcommand git fsmonitor--daemon start <super-sub.trace
     - '
     - 
      +test_expect_success "submodule implicitly starts daemon by pull" '
     -+	test_atexit "stop_git_and_watchdog" &&
     -+	test_when_finished "rm -rf cloned; \
     -+			    rm -rf super; \
     -+			    rm -rf sub" &&
     ++	test_atexit "stop_watchdog" &&
     ++	test_when_finished "stop_git && rm -rf cloned super sub" &&
      +
      +	create_super super &&
      +	create_sub sub &&
     @@ t/t7527-builtin-fsmonitor.sh: test_expect_success "submodule absorbgitdirs impli
      +	git clone --recurse-submodules super cloned &&
      +
      +	git -C cloned/dir_1/dir_2/sub config core.fsmonitor true &&
     ++	set -m &&
      +	start_git_in_background -C cloned pull --recurse-submodules
      +'
      +

 builtin/fsmonitor--daemon.c  |  1 +
 t/t7527-builtin-fsmonitor.sh | 51 ++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

builtin/fsmonitor--daemon.c +1 -0

index dce8a3b2482..e1e6b96d09e 100644
@@ -1314,6 +1314,7 @@ static int fsmonitor_run_daemon(void)
 		strbuf_reset(&state.path_gitdir_watch);
 		strbuf_addstr(&state.path_gitdir_watch,
 			      absolute_path(repo_get_git_dir(the_repository)));
+		strbuf_strip_suffix(&state.path_gitdir_watch, "/.");
 		state.nr_paths_watching = 2;
 	}

t/t7527-builtin-fsmonitor.sh +51 -0

57 lines not on this page · raw message

base-commit: 3857aae53f3633b7de63ad640737c657387ae0c6

Signature · 1 line

gitgitgadget

Junio C Hamano

2024-10-01 11:57 UTC #4 lore raw

Quoted 16 lines · Koji Nakamaru via GitGitGadget

From: Koji Nakamaru <[email protected]>
fsmonitor_classify_path_absolute() expects state->path_gitdir_watch.buf has no trailing '/' or '.' For a submodule, fsmonitor_run_daemon() sets the value with trailing "/." (as repo_get_git_dir(the_repository) on Darwin returns ".") so that fsmonitor_classify_path_absolute() returns IS_OUTSIDE_CONE.
In this case, fsevent_callback() doesn't update cookie_list so that fsmonitor_publish() does nothing and with_lock__mark_cookies_seen() is not invoked.
As with_lock__wait_for_cookie() infinitely waits for state->cookies_cond that with_lock__mark_cookies_seen() should unlock, the whole daemon hangs.

Quoted 5 lines

Remove trailing "/." from state->path_gitdir_watch.buf for submodules and add a corresponding test in t7527-builtin-fsmonitor.sh.
Suggested-by Johannes Schindelin <[email protected]>
Suggested-by Junio C Hamano <[email protected]>

In none of the changes described above, I have any input to deserve such credit, though.

Quoted 16 lines

t/t7527-builtin-fsmonitor.sh +16 -0

+start_git_in_background () {
+	git "$@" &
+	git_pid=$!
+	git_pgid=$(ps -o pgid= -p $git_pid)
+	nr_tries_left=10
+	while true
+	do
+		if test $nr_tries_left -eq 0
+		then
+			kill -- -$git_pgid
+			exit 1
+		fi
+		sleep 1
+		nr_tries_left=$(($nr_tries_left - 1))
+	done >/dev/null 2>&1 &
+	watchdog_pid=$!

t/t7527-builtin-fsmonitor.sh test_expect_success "submodule absorbgitdirs implicitly starts daemon" ' · line 935

Quoted 10 lines

t/t7527-builtin-fsmonitor.sh +10 -0

+	wait $git_pid
+}
+
+stop_git () {
+	while kill -0 -- -$git_pgid
+	do
+		kill -- -$git_pgid
+		sleep 1
+	done
+}

On the "git" side you use process group because you expect that "git" would spawn subprocesses and you want to catch all of them,
...

t/t7527-builtin-fsmonitor.sh test_expect_success "submodule absorbgitdirs implicitly starts daemon" ' · line 943

Quoted 7 lines

t/t7527-builtin-fsmonitor.sh +7 -0

+stop_watchdog () {
+	while kill -0 $watchdog_pid
+	do
+		kill $watchdog_pid
+		sleep 1
+	done
+}

... but "watchdog" you know is a single process, so you'd only need a single process id, is that the idea?

What is the motivation behind the change in this iteration to use process group? Was it observed that leftover processes hang around if we killed only the $git_pid, or something?

t/t7527-builtin-fsmonitor.sh test_expect_success "submodule absorbgitdirs implicitly starts daemon" ' · line 947

Quoted 3 lines

t/t7527-builtin-fsmonitor.sh +3 -0

+test_expect_success "submodule implicitly starts daemon by pull" '
+	test_atexit "stop_watchdog" &&
+	test_when_finished "stop_git && rm -rf cloned super sub" &&

If stop_git ever returns with non-zero status, "rm -rf" will be skipped, which I am not sure is a good idea.

The whole test_when_finished would fail in such a case, so you would notice the problem right away, which is a plus, though.

t/t7527-builtin-fsmonitor.sh test_expect_success "submodule absorbgitdirs implicitly starts daemon" ' · line 957

Quoted 9 lines

t/t7527-builtin-fsmonitor.sh +9 -0

+	create_super super &&
+	create_sub sub &&
+
+	git -C super submodule add ../sub ./dir_1/dir_2/sub &&
+	git -C super commit -m "add sub" &&
+	git clone --recurse-submodules super cloned &&
+
+	git -C cloned/dir_1/dir_2/sub config core.fsmonitor true &&
+	set -m &&

I have to wonder how portable (and necessary) this is.

POSIX says it shall be supported if the implementation supports the User Portability Utilities option. It also says that it was added to apply only to the UPE because it applies primarily to interactive use, not shell script applications. And our test scripts are of course not interactive.

Thanks.

Koji Nakamaru

2024-10-01 17:51 UTC #5 lore raw

Quoted 14 lines · Junio C Hamano

Quoted 14 lines
fsmonitor_classify_path_absolute() expects state->path_gitdir_watch.buf has no trailing '/' or '.' For a submodule, fsmonitor_run_daemon() sets the value with trailing "/." (as repo_get_git_dir(the_repository) on Darwin returns ".") so that fsmonitor_classify_path_absolute() returns IS_OUTSIDE_CONE.
In this case, fsevent_callback() doesn't update cookie_list so that fsmonitor_publish() does nothing and with_lock__mark_cookies_seen() is not invoked.
As with_lock__wait_for_cookie() infinitely waits for state->cookies_cond that with_lock__mark_cookies_seen() should unlock, the whole daemon hangs.

Quoted 8 lines

Quoted 5 lines
Remove trailing "/." from state->path_gitdir_watch.buf for submodules and add a corresponding test in t7527-builtin-fsmonitor.sh.
Suggested-by Johannes Schindelin <[email protected]>
Suggested-by Junio C Hamano <[email protected]>
In none of the changes described above, I have any input to deserve such credit, though.

Your points are very helpful :)

Quoted 31 lines

Quoted 16 lines
t/t7527-builtin-fsmonitor.sh +16 -0
+start_git_in_background () {
+ git "$@" &
+ git_pid=$!
+ git_pgid=$(ps -o pgid= -p $git_pid)
+ nr_tries_left=10
+ while true
+ do
+ if test $nr_tries_left -eq 0
+ then
+ kill -- -$git_pgid
+ exit 1
+ fi
+ sleep 1
+ nr_tries_left=$(($nr_tries_left - 1))
+ done >/dev/null 2>&1 &
+ watchdog_pid=$!
t/t7527-builtin-fsmonitor.sh test_expect_success "submodule absorbgitdirs implicitly starts daemon" ' · line 935
Quoted 10 lines
t/t7527-builtin-fsmonitor.sh +10 -0
+ wait $git_pid
+}
+
+stop_git () {
+ while kill -0 -- -$git_pgid
+ do
+ kill -- -$git_pgid
+ sleep 1
+ done
+}
On the "git" side you use process group because you expect that "git" would spawn subprocesses and you want to catch all of them,
...

Quoted 10 lines

t/t7527-builtin-fsmonitor.sh test_expect_success "submodule absorbgitdirs implicitly starts daemon" ' · line 943
Quoted 7 lines
t/t7527-builtin-fsmonitor.sh +7 -0
+stop_watchdog () {
+ while kill -0 $watchdog_pid
+ do
+ kill $watchdog_pid
+ sleep 1
+ done
+}
... but "watchdog" you know is a single process, so you'd only need a single process id, is that the idea?

Yes, that is the idea.

Quoted 3 lines

What is the motivation behind the change in this iteration to use process group? Was it observed that leftover processes hang around if we killed only the $git_pid, or something?

Yes, if the issue occurs, three processes remains:

  git fetch --update-head-ok --recurse-submodules=on

  git fetch --no-prune --no-prune-tags --update-head-ok
    --recurse-submodules --recurse-submodules-default yes
    --submodule-prefix=dir_1/dir_2/sub/

  git fsmonitor--daemon run --detach --ipc-threads=8

If there is no issue, only the fsmonitor process remains.

Quoted 9 lines

Quoted 3 lines
+test_expect_success "submodule implicitly starts daemon by pull" '
+ test_atexit "stop_watchdog" &&
+ test_when_finished "stop_git && rm -rf cloned super sub" &&
If stop_git ever returns with non-zero status, "rm -rf" will be skipped, which I am not sure is a good idea.
The whole test_when_finished would fail in such a case, so you would notice the problem right away, which is a plus, though.

t/README discusses that test_when_finished and test_atexit differ about the "--immediate" option. As git and its subprocesses are the test target, I moved stop_git to the current place. This might be however confusing when someone later reads this test. Should we simply put stop_git and stop_watchdong in test_atexit?

Quoted 12 lines

t/t7527-builtin-fsmonitor.sh test_expect_success "submodule absorbgitdirs implicitly starts daemon" ' · line 957
Quoted 9 lines
+ create_super super &&
+ create_sub sub &&
t/t7527-builtin-fsmonitor.sh +7 -0
+
+ git -C super submodule add ../sub ./dir_1/dir_2/sub &&
+ git -C super commit -m "add sub" &&
+ git clone --recurse-submodules super cloned &&
+
+ git -C cloned/dir_1/dir_2/sub config core.fsmonitor true &&
+ set -m &&
I have to wonder how portable (and necessary) this is.

Quoted 5 lines

POSIX says it shall be supported if the implementation supports the User Portability Utilities option. It also says that it was added to apply only to the UPE because it applies primarily to interactive use, not shell script applications. And our test scripts are of course not interactive.

How about the following modification? It still utilizes $git_pgid to filter processes, but avoids "set -m".

  diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
  index 2dd1ca1a7b..23d9a7c953 100755
  --- a/t/t7527-builtin-fsmonitor.sh
  +++ b/t/t7527-builtin-fsmonitor.sh
  @@ -916,7 +916,7 @@ start_git_in_background () {
          do
                  if test $nr_tries_left -eq 0
                  then
  -                       kill -- -$git_pgid
  +                       kill $git_pid
                          exit 1
                  fi
                  sleep 1
  @@ -927,10 +927,13 @@ start_git_in_background () {
   }

stop_git () {
- while kill -0 -- -$git_pgid
+ for p in $(ps -o pgid=,pid=,comm= | grep "^$git_pgid .*git"
| sed 's/^[0-9][0-9]* $[0-9][0-9]*$ .*/\1/')
do

  -               kill -- -$git_pgid
  -               sleep 1
  +               while kill -0 $p
  +               do

+ kill $p
+ sleep 1
+ done
done
}

@@ -954,7 +957,6 @@ test_expect_success "submodule implicitly starts
daemon by pull" '

          git clone --recurse-submodules super cloned &&

          git -C cloned/dir_1/dir_2/sub config core.fsmonitor true &&
  -       set -m &&
          start_git_in_background -C cloned pull --recurse-submodules

Koji Nakamaru

Junio C Hamano

2024-10-01 18:04 UTC #6 lore raw

Quoted 7 lines · Koji Nakamaru

Quoted 7 lines
Quoted 3 lines
+test_expect_success "submodule implicitly starts daemon by pull" '
+ test_atexit "stop_watchdog" &&
+ test_when_finished "stop_git && rm -rf cloned super sub" &&
If stop_git ever returns with non-zero status, "rm -rf" will be skipped, which I am not sure is a good idea.

Quoted 8 lines

Quoted 2 lines
The whole test_when_finished would fail in such a case, so you would notice the problem right away, which is a plus, though.
t/README discusses that test_when_finished and test_atexit differ about the "--immediate" option. As git and its subprocesses are the test target, I moved stop_git to the current place. This might be however confusing when someone later reads this test. Should we simply put stop_git and stop_watchdong in test_atexit?

That is not what I meant.

I was merely questioning the &&-chaining that stops "rm -fr" from running if stop_git ever fails (and your earlier iteration you had multiple "rm -fr" ;-chained, not &&-chained---not using && is often more appropriate in a when_finished handler).

Quoted 4 lines

Quoted 4 lines
Quoted 1 line
+ set -m &&
I have to wonder how portable (and necessary) this is.

Quoted 8 lines

Quoted 5 lines
POSIX says it shall be supported if the implementation supports the User Portability Utilities option. It also says that it was added to apply only to the UPE because it applies primarily to interactive use, not shell script applications. And our test scripts are of course not interactive.
How about the following modification? It still utilizes $git_pgid to filter processes, but avoids "set -m".

Nah, your original reads much better, and the code is grabbing and using the process group information anyway (and my question about "-m" was more about "should we be relying on process group features in this test to kill them all?").

I am OK with the idea that we can assume, at least among the platforms that support fsmonitor, that sending a signal to a process group would cause the signal delivered to the member processes just as we expect.

Thanks.

Koji Nakamaru

2024-10-01 18:42 UTC #7 lore raw

Quoted 16 lines · Junio C Hamano

Quoted 7 lines
Quoted 7 lines
Quoted 3 lines
+test_expect_success "submodule implicitly starts daemon by pull" '
+ test_atexit "stop_watchdog" &&
+ test_when_finished "stop_git && rm -rf cloned super sub" &&
If stop_git ever returns with non-zero status, "rm -rf" will be skipped, which I am not sure is a good idea.
Quoted 8 lines
Quoted 2 lines
The whole test_when_finished would fail in such a case, so you would notice the problem right away, which is a plus, though.
t/README discusses that test_when_finished and test_atexit differ about the "--immediate" option. As git and its subprocesses are the test target, I moved stop_git to the current place. This might be however confusing when someone later reads this test. Should we simply put stop_git and stop_watchdong in test_atexit?

Quoted 6 lines

That is not what I meant.
I was merely questioning the &&-chaining that stops "rm -fr" from running if stop_git ever fails (and your earlier iteration you had multiple "rm -fr" ;-chained, not &&-chained---not using && is often more appropriate in a when_finished handler).

I see. I'll fix this part.

Quoted 18 lines

Quoted 4 lines
Quoted 4 lines
Quoted 1 line
+ set -m &&
I have to wonder how portable (and necessary) this is.
Quoted 8 lines
Quoted 5 lines
POSIX says it shall be supported if the implementation supports the User Portability Utilities option. It also says that it was added to apply only to the UPE because it applies primarily to interactive use, not shell script applications. And our test scripts are of course not interactive.
How about the following modification? It still utilizes $git_pgid to filter processes, but avoids "set -m".
Nah, your original reads much better, and the code is grabbing and using the process group information anyway (and my question about "-m" was more about "should we be relying on process group features in this test to kill them all?").

Quoted 4 lines

I am OK with the idea that we can assume, at least among the platforms that support fsmonitor, that sending a signal to a process group would cause the signal delivered to the member processes just as we expect.

Thank you for the clarification and the support.

Koji Nakamaru

Koji Nakamaru via GitGitGadget

author

2024-10-01 19:29 UTC #13 lore raw

PATCH v3 1/1 fsmonitor OSX: fix hangs for submodules

In reply to Koji Nakamaru via GitGitGadget

From: Koji Nakamaru <[email protected]>

In this case, fsevent_callback() doesn't update cookie_list so that fsmonitor_publish() does nothing and with_lock__mark_cookies_seen() is not invoked.

As with_lock__wait_for_cookie() infinitely waits for state->cookies_cond that with_lock__mark_cookies_seen() should unlock, the whole daemon hangs.

Remove trailing "/." from state->path_gitdir_watch.buf for submodules and add a corresponding test in t7527-builtin-fsmonitor.sh.

Signed-off-by Koji Nakamaru <[email protected]>

---
fsmonitor/darwin: fix hangs for submodules

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1802%2FKojiNakamaru%2Ffix%2Ffsmonitor-darwin-hangs-for-submodules-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1802/KojiNakamaru/fix/fsmonitor-darwin-hangs-for-submodules-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1802

Range-diff vs v2:

1: decf68499f7 ! 1: aabc1c2f6ee fsmonitor OSX: fix hangs for submodules

     @@ t/t7527-builtin-fsmonitor.sh: test_expect_success "submodule absorbgitdirs impli
      +
      +test_expect_success "submodule implicitly starts daemon by pull" '
      +	test_atexit "stop_watchdog" &&
     -+	test_when_finished "stop_git && rm -rf cloned super sub" &&
     ++	test_when_finished "stop_git; rm -rf cloned super sub" &&
      +
      +	create_super super &&
      +	create_sub sub &&

 builtin/fsmonitor--daemon.c  |  1 +
 t/t7527-builtin-fsmonitor.sh | 51 ++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

builtin/fsmonitor--daemon.c +1 -0

index dce8a3b2482..e1e6b96d09e 100644
@@ -1314,6 +1314,7 @@ static int fsmonitor_run_daemon(void)
 		strbuf_reset(&state.path_gitdir_watch);
 		strbuf_addstr(&state.path_gitdir_watch,
 			      absolute_path(repo_get_git_dir(the_repository)));
+		strbuf_strip_suffix(&state.path_gitdir_watch, "/.");
 		state.nr_paths_watching = 2;
 	}

t/t7527-builtin-fsmonitor.sh +51 -0

57 lines not on this page · raw message

base-commit: 3857aae53f3633b7de63ad640737c657387ae0c6

Signature · 1 line

gitgitgadget

Koji Nakamaru

2024-10-02 06:58 UTC #8 lore raw

In reply to Koji Nakamaru

Although I submitted [PATCH v3], it was incomplete about the following:

Quoted 4 lines · Junio C Hamano

I am OK with the idea that we can assume, at least among the platforms that support fsmonitor, that sending a signal to a process group would cause the signal delivered to the member processes just as we expect.

On windows, there is no process group so the test cannot run correctly. As hangs corrected with the patch occur only for darwin, I would like to skip MINGW in the test. Is it okay?

Koji Nakamaru

Junio C Hamano

2024-10-02 18:14 UTC #9 lore raw

Quoted 3 lines · Koji Nakamaru

On windows, there is no process group so the test cannot run correctly. As hangs corrected with the patch occur only for darwin, I would like to skip MINGW in the test. Is it okay?

Surely. But can we do so without spelling MINGW or WINDOWS out?

That is, if your test requires process group features available, can we come up with a lazy prerequisite and use that to decide if we skip the test?

Earlier in the discussion, you said who are left behind if we did so on systems with process groups, but I wonder what happens when we throw a signal at the top-level "git" process on Windows, and if it behaves better, perhaps we can implement stop_git differently where process groups are not available, instead of skipping the tests altogether?

Thanks.

Koji Nakamaru

2024-10-03 08:55 UTC #10 lore raw

Quoted 9 lines · Junio C Hamano

Quoted 3 lines
On windows, there is no process group so the test cannot run correctly. As hangs corrected with the patch occur only for darwin, I would like to skip MINGW in the test. Is it okay?
Surely. But can we do so without spelling MINGW or WINDOWS out?
That is, if your test requires process group features available, can we come up with a lazy prerequisite and use that to decide if we skip the test?

I tweaked fsm-listen-win32.c to cause hangs and tested on windows. I'm sorry that simply saying "there is no process group" was not quite correct. Each mingw process has a process group and its win32 subprocesses can be killed by "kill -- -$pgid"

For example, when a hang occurs, the following processes remain.

      PID    PPID    PGID     WINPID   TTY         UID    STIME
        COMMAND
    56782   40923   56782       9484  pty0     1052296 16:23:22
        /mingw64/bin/git
        # mingw git process
    78100       0       0      12564  ?              0 16:23:23
        C:\git-sdk-64\mingw64\libexec\git-core\git.exe
        # win32 process
    86108       0       0      20572  ?              0 16:23:23
        C:\git-sdk-64\mingw64\libexec\git-core\git.exe
        # win32 subprocess
    73328       0       0       7792  ?              0 16:23:23
        C:\git-sdk-64\mingw64\libexec\git-core\git.exe
        # win32 fsmonitor

Quoted 6 lines

Earlier in the discussion, you said who are left behind if we did so on systems with process groups, but I wonder what happens when we throw a signal at the top-level "git" process on Windows, and if it behaves better, perhaps we can implement stop_git differently where process groups are not available, instead of skipping the tests altogether?

If we do "kill 56782" or "kill -- -56782" for the above example, most of processes are terminated except the win32 fsmonitor. This is because the win32 fsmonitor is detached by FreeConsole().

I also tried "git fsmonitor--daemon stop". It was able to communicate with the win32 fsmonitor and the internal status of the win32 fsmonitor changed, but the win32 daemon didn't terminate.

Because it's getting complicated, how about the following:

* specify MINGW
* note in the commit log:
The test is disabled for MINGW because hangs treated with this patch occur only for Darwin and there is no simple way to terminate the win32 fsmonitor daemon that hangs.

Koji Nakamaru

Junio C Hamano

2024-10-03 17:44 UTC #11 lore raw

Quoted 7 lines · Koji Nakamaru

Because it's getting complicated, how about the following:
* specify MINGW
* note in the commit log:
The test is disabled for MINGW because hangs treated with this patch occur only for Darwin and there is no simple way to terminate the win32 fsmonitor daemon that hangs.

Sounds good to me.

Thanks.

Koji Nakamaru via GitGitGadget

author

2024-10-04 00:07 UTC #14 lore raw

PATCH v4 1/1 fsmonitor OSX: fix hangs for submodules

In reply to Koji Nakamaru via GitGitGadget

From: Koji Nakamaru <[email protected]>

In this case, fsevent_callback() doesn't update cookie_list so that fsmonitor_publish() does nothing and with_lock__mark_cookies_seen() is not invoked.

As with_lock__wait_for_cookie() infinitely waits for state->cookies_cond that with_lock__mark_cookies_seen() should unlock, the whole daemon hangs.

Remove trailing "/." from state->path_gitdir_watch.buf for submodules and add a corresponding test in t7527-builtin-fsmonitor.sh. The test is disabled for MINGW because hangs treated with this patch occur only for Darwin and there is no simple way to terminate the win32 fsmonitor daemon that hangs.

Signed-off-by Koji Nakamaru <[email protected]>

---
fsmonitor/darwin: fix hangs for submodules

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1802%2FKojiNakamaru%2Ffix%2Ffsmonitor-darwin-hangs-for-submodules-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1802/KojiNakamaru/fix/fsmonitor-darwin-hangs-for-submodules-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1802

Range-diff vs v3:

1: aabc1c2f6ee ! 1: 7b7224ef718 fsmonitor OSX: fix hangs for submodules
@@ Commit message
hangs.

Remove trailing "/." from state->path_gitdir_watch.buf for submodules
- and add a corresponding test in t7527-builtin-fsmonitor.sh.
+ and add a corresponding test in t7527-builtin-fsmonitor.sh. The test is
+ disabled for MINGW because hangs treated with this patch occur only for
+ Darwin and there is no simple way to terminate the win32 fsmonitor
+ daemon that hangs.

@@ t/t7527-builtin-fsmonitor.sh: test_expect_success "submodule absorbgitdirs impli
+ done
+}
+
-+test_expect_success "submodule implicitly starts daemon by pull" ' ++test_expect_success !MINGW "submodule implicitly starts daemon by pull" '
+ test_atexit "stop_watchdog" &&
+ test_when_finished "stop_git; rm -rf cloned super sub" &&
+

 builtin/fsmonitor--daemon.c  |  1 +
 t/t7527-builtin-fsmonitor.sh | 51 ++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

builtin/fsmonitor--daemon.c +1 -0

index dce8a3b2482..e1e6b96d09e 100644
@@ -1314,6 +1314,7 @@ static int fsmonitor_run_daemon(void)
 		strbuf_reset(&state.path_gitdir_watch);
 		strbuf_addstr(&state.path_gitdir_watch,
 			      absolute_path(repo_get_git_dir(the_repository)));
+		strbuf_strip_suffix(&state.path_gitdir_watch, "/.");
 		state.nr_paths_watching = 2;
 	}

t/t7527-builtin-fsmonitor.sh +51 -0

57 lines not on this page · raw message

base-commit: 3857aae53f3633b7de63ad640737c657387ae0c6

Signature · 1 line

gitgitgadget

Koji Nakamaru

2024-10-04 02:42 UTC #12 lore raw

In reply to Junio C Hamano

Quoted 9 lines · Junio C Hamano

Quoted 7 lines
Because it's getting complicated, how about the following:
* specify MINGW
* note in the commit log:
The test is disabled for MINGW because hangs treated with this patch occur only for Darwin and there is no simple way to terminate the win32 fsmonitor daemon that hangs.
Sounds good to me.

Thank you. I've submitted [PATCH v4].

Koji Nakamaru

Junio C Hamano

2024-10-04 17:44 UTC #15 lore raw

In reply to Koji Nakamaru via GitGitGadget

Quoted 5 lines · Koji Nakamaru via GitGitGadget

... The test is
disabled for MINGW because hangs treated with this patch occur only for Darwin and there is no simple way to terminate the win32 fsmonitor daemon that hangs.
...

Quoted 9 lines

     @@ t/t7527-builtin-fsmonitor.sh: test_expect_success "submodule absorbgitdirs impli
      +	done
      +}
      +
     -+test_expect_success "submodule implicitly starts daemon by pull" '
     ++test_expect_success !MINGW "submodule implicitly starts daemon by pull" '
      +	test_atexit "stop_watchdog" &&
      +	test_when_finished "stop_git; rm -rf cloned super sub" &&
      +

Let me update !MINGW to !WINDOWS while queuing.

Ramsay Jones

2024-10-04 18:48 UTC #16 lore raw

Quoted 17 lines · Junio C Hamano

Quoted 5 lines · Koji Nakamaru via GitGitGadget
... The test is
disabled for MINGW because hangs treated with this patch occur only for Darwin and there is no simple way to terminate the win32 fsmonitor daemon that hangs.
...
Quoted 9 lines
     @@ t/t7527-builtin-fsmonitor.sh: test_expect_success "submodule absorbgitdirs impli
      +	done
      +}
      +
     -+test_expect_success "submodule implicitly starts daemon by pull" '
     ++test_expect_success !MINGW "submodule implicitly starts daemon by pull" '
      +	test_atexit "stop_watchdog" &&
      +	test_when_finished "stop_git; rm -rf cloned super sub" &&
      +

Quoted 2 lines

Let me update !MINGW to !WINDOWS while queuing.

While this won't hurt, this test file is skipped on cygwin:

[23:19:33] t7527-builtin-fsmonitor.sh ......................... skipped: fsmonitor--daemon is not supported on this platform

(my eternal TODO list has an 'fsmonitor on cygwin?' item ...)

Thanks.

ATB,
Ramsay Jones

Junio C Hamano

2024-10-04 19:07 UTC #17 lore raw

Quoted 5 lines · Ramsay Jones

While this won't hurt, this test file is skipped on cygwin:
[23:19:33] t7527-builtin-fsmonitor.sh ......................... skipped: fsmonitor--daemon is not supported on this platform
(my eternal TODO list has an 'fsmonitor on cygwin?' item ...)

Thanks, then let's not bother.

Koji Nakamaru

2024-10-05 03:13 UTC #18 lore raw

In reply to Junio C Hamano

Quoted 15 lines · Junio C Hamano

Quoted 6 lines
... The test is
disabled for MINGW because hangs treated with this patch occur only for Darwin and there is no simple way to terminate the win32 fsmonitor
daemon that hangs.
...
@@ t/t7527-builtin-fsmonitor.sh: test_expect_success "submodule absorbgitdirs impli
Quoted 8 lines
+ done
+}
+
-+test_expect_success "submodule implicitly starts daemon by pull" ' ++test_expect_success !MINGW "submodule implicitly starts daemon by pull" '
+ test_atexit "stop_watchdog" &&
+ test_when_finished "stop_git; rm -rf cloned super sub" &&
+

Quoted 1 line

Let me update !MINGW to !WINDOWS while queuing.

I see, Thank you.

Koji Nakamaru