[PATCH 0/2] alternate approach to fixing fsmonitor hangs
- From
Jeff King <peff@peff.net>
- Date
- Oct 8, 2024, 08:31 UTC
- Message-ID
- <20241008083121.GA676391@coredump.intra.peff.net>
- In-Reply-To
- <CAOTNsDwwikiX3u6DG=+4hn+mcgfXzzDoqR3ZFVEdGi=mPGQbpg@mail.gmail.com>
On Mon, Oct 07, 2024 at 06:45:22PM +0900, Koji Nakamaru wrote:
Show 8 quoted lines
> > - 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?
Yes, good catch.
Show 5 quoted lines
> > 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.
Yeah, I never even tried building mine on Windows, and was just testing via tmate on our macOS CI environment. :-/
I've added in the necessary Windows bits, along with smoothing a few rough edges. Especially with the extra Windows changes (which I mostly had to guess-and-check by pushing to CI), I'm beginning to wonder if my solution isn't getting a bit too complicated, and maybe yours was the right way after all.
But I've cleaned it up for presentation here, so at least we can look at the final form of both and see which we prefer.
[1/2]: simple-ipc: split async server initialization and running [2/2]: fsmonitor: initialize fs event listener before accepting clients
builtin/fsmonitor--daemon.c | 6 ++-- compat/fsmonitor/fsm-listen-darwin.c | 6 ++++ compat/fsmonitor/fsm-listen-win32.c | 6 ++++ compat/simple-ipc/ipc-shared.c | 5 +-- compat/simple-ipc/ipc-unix-socket.c | 28 +++++++++++++--- compat/simple-ipc/ipc-win32.c | 48 +++++++++++++++++++++++++--- simple-ipc.h | 17 +++++++--- 7 files changed, 98 insertions(+), 18 deletions(-)
-Peff