Re: [PATCH 18/19] index-helper: autorun

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mar 17, 2016, 14:43 UTC
Message-ID: <alpine.DEB.2.20.1603171536420.4690@virtualbox>
In-Reply-To: <CACsJy8AsJKmsPm8Y1LRZdmyH60n3OT5X=42RGK5GXNBDfn8j8g@mail.gmail.com>

Hi Duy,

On Thu, 17 Mar 2016, Duy Nguyen wrote:

Show 9 quoted lines

> On Thu, Mar 17, 2016 at 1:27 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > I am much more concerned about concurrent accesses and the communication
> > between the Git processes and the index-helper. Writing to the .pid file
> > sounds very fragile to me, in particular when multiple processes can poke
> > the index-helper in succession and some readers are unaware that the index
> > is being refreshed.
> 
> It's not that bad.

Well, the way I read the code it is possible that:

1. Git process 1 starts, reading the index
2. Git process 2 starts, poking the index-helper
3. The index-helper updates the .pid file (why not set a bit in the shared
   memory?) with a prefix "W"
4. Git process 2 reads the .pid file and waits for the "W" to go away
   (what if index-helper is not fast enough to write the "W"?)
5. Git process 1 access the index, happily oblivious that it is being
   updated and the data is in an inconsistent state

> We should have protection in place to deal with this and fall back to
> reading directly from file when things get suspicious.

I really want to prevent that. I know of use cases where the index weighs 300MB, and falling back to reading it directly *really* hurts.

> But I agree that sending UNIX signals (or PostMessage) is not really
> good communication.

Yeah, I really would like two-way communication instead. Named pipes? They'd have the advantage that you could use the full path to the index as identifier.

The way I read the current code, we would actually create a different shared memory every time the index changes because its checksum is part of the shared memory's "path"...

Ciao, Dscho