Re: [PATCH] fsmonitor: fix hangs by delayed fs event listening

From: Koji Nakamaru <koji.nakamaru@gree.net>
Date: Oct 7, 2024, 09:45 UTC
Message-ID: <CAOTNsDwwikiX3u6DG=+4hn+mcgfXzzDoqR3ZFVEdGi=mPGQbpg@mail.gmail.com>
In-Reply-To: <20241007060813.GA34827@coredump.intra.peff.net>

On Mon, Oct 07, 2024 at 01:58:21AM -0400, Jeff King wrote:

> First off, thank you for looking into this. I _think_ what you have here
> would work, but I had envisioned something a little different. So let me
> first try to walk through your solution...

Thank you very much for looking through my patch in detail and providing another approach. I agree that busy-waiting is not smart ;) I utilized it to minimize code modification and not to worry about any new deadlock. Your approach is more natural if the code is written from scratch with the problem in mind.

Show 10 quoted lines

> @@ -933,7 +949,9 @@ int ipc_server_stop_async(struct ipc_server_data *server_data)
>
>       trace2_region_enter("ipc-server", "server-stop-async", NULL);
>
> -     pthread_mutex_lock(&server_data->work_available_mutex);
> +     /* If we haven't started yet, we are already holding lock. */
> +     if (!server_data->started)
> +             pthread_mutex_lock(&server_data->work_available_mutex);
>
>       server_data->shutdown_requested = 1;

Is this condition inverted?

On Mon, Oct 7, 2024 at 3:08 PM Jeff King <peff@peff.net> wrote:

Show 7 quoted lines

> I just checked your patch in our CI, using the sleep(1) you suggested
> earlier to more predictably lose the race(). It does work reliably (and
> I confirmed with some extra trace statements that it does spin on the
> sleep_millisec() loop).
>
> I had also previously checked my suggested solution. So I do think
> either is a valid solution to the problem.

I also tested your approach on Windows with a few additions to ipc-win32.c, and it worked correctly.

* define work_available_mutex and started in ipc_server_data.

* call pthread_mutex_lock(&server_data->work_available_mutex) before
creating the server_thread_proc thread.

* define ipc_server_start()

Shall we adopt your approach as it is more natural. Can I ask you to submit a new patch?

Koji Nakamaru