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.
> @@ -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@xxxxxxxx> wrote:
> 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
