Bug: fatal: Unable to create '.../.git/index.lock': File exists.

Bug: fatal: Unable to create '.../.git/index.lock': File exists.

[Aleksey Midenkov]

Reproduce:
```
cat << EOF >> /tmp/check.sh
#!/bin/sh
git log HEAD~..HEAD | cat
# sleep 1
EOF
chmod +x /tmp/check.sh
git rebase -p -x /tmp/check.sh base
```
If the `base` is far away enough it fails with "fatal: Unable to
create '.../.git/index.lock': File exists." at an arbitrary commit.

Abort current rebase, uncomment 'sleep 1' and repeat rebase. Now it
doesn't fail.

Version:
git version 2.20.1

uname --all
Linux lian 4.18.0-15-generic #16-Ubuntu SMP Thu Feb 7 10:56:39 UTC
2019 x86_64 x86_64 x86_64 GNU/Linux


-- 
All the best,

Aleksey Midenkov
@midenok

Re: Bug: fatal: Unable to create '.../.git/index.lock': File exists.

[Duy Nguyen]

On Mon, Apr 29, 2019 at 6:03 PM Aleksey Midenkov <midenok@gmail.com> wrote:
>
> Reproduce:
> ```
> cat << EOF >> /tmp/check.sh
> #!/bin/sh
> git log HEAD~..HEAD | cat
> # sleep 1
> EOF
> chmod +x /tmp/check.sh
> git rebase -p -x /tmp/check.sh base
> ```
> If the `base` is far away enough it fails with "fatal: Unable to
> create '.../.git/index.lock': File exists." at an arbitrary commit.

I gave it about 2000 commits (from v2.20.1 to master on git.git) to
rebase. No luck.

This --preserve-merges is being completely replaced in latest git
version though. Is it possible for you to try master branch of git.git
and see if the problem is still there?

>
> Abort current rebase, uncomment 'sleep 1' and repeat rebase. Now it
> doesn't fail.
>
> Version:
> git version 2.20.1
>
> uname --all
> Linux lian 4.18.0-15-generic #16-Ubuntu SMP Thu Feb 7 10:56:39 UTC
> 2019 x86_64 x86_64 x86_64 GNU/Linux
>
>
> --
> All the best,
>
> Aleksey Midenkov
> @midenok



-- 
Duy

Re: Bug: fatal: Unable to create '.../.git/index.lock': File exists.

[Johannes Schindelin]

Hi Aleksey,

On Mon, 29 Apr 2019, Aleksey Midenkov wrote:

> git rebase -p -x /tmp/check.sh base
> ```
> If the `base` is far away enough it fails with "fatal: Unable to
> create '.../.git/index.lock': File exists." at an arbitrary commit.

Does it work if you pass `-r` instead of `-p`? The latter will be
deprecated in favor of the former in the upcoming Git v2.22.

Ciao,
Johannes

Re: Bug: fatal: Unable to create '.../.git/index.lock': File exists.

[Aleksey Midenkov]

On Mon, Apr 29, 2019 at 2:35 PM Duy Nguyen <pclouds@gmail.com> wrote:
>
> On Mon, Apr 29, 2019 at 6:03 PM Aleksey Midenkov <midenok@gmail.com> wrote:
> >
> > Reproduce:
> > ```
> > cat << EOF >> /tmp/check.sh
> > #!/bin/sh
> > git log HEAD~..HEAD | cat
> > # sleep 1
> > EOF
> > chmod +x /tmp/check.sh
> > git rebase -p -x /tmp/check.sh base
> > ```
> > If the `base` is far away enough it fails with "fatal: Unable to
> > create '.../.git/index.lock': File exists." at an arbitrary commit.
>
> I gave it about 2000 commits (from v2.20.1 to master on git.git) to
> rebase. No luck.

Please, try on this repo: git@github.com:tempesta-tech/mariadb

```
git checkout 62a082f573
git rebase -p -x /tmp/check.sh ca7fbcea6c4
```

On Tue, Apr 30, 2019 at 12:10 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
...
>
> Does it work if you pass `-r` instead of `-p`? The latter will be
> deprecated in favor of the former in the upcoming Git v2.22.

It also fails with `-r` but less frequently.

-- 
All the best,

Aleksey Midenkov
@midenok

Re: Bug: fatal: Unable to create '.../.git/index.lock': File exists.

[Jeff King]

On Tue, Apr 30, 2019 at 02:19:11PM +0300, Aleksey Midenkov wrote:

> > I gave it about 2000 commits (from v2.20.1 to master on git.git) to
> > rebase. No luck.
> 
> Please, try on this repo: git@github.com:tempesta-tech/mariadb
> 
> ```
> git checkout 62a082f573
> git rebase -p -x /tmp/check.sh ca7fbcea6c4
> ```

It doesn't reproduce for me.

Usually when we see racy contention on index.lock, the culprit turns out
to be another unrelated git process refreshing the index. Do you have
anything else running which might be using "git status" (e.g., magit in
emacs, vim git integration, etc)?

-Peff

Re: Bug: fatal: Unable to create '.../.git/index.lock': File exists.

[Aleksey Midenkov]

On Tue, Apr 30, 2019 at 8:41 PM Jeff King <peff@peff.net> wrote:
>
> On Tue, Apr 30, 2019 at 02:19:11PM +0300, Aleksey Midenkov wrote:
>
> > > I gave it about 2000 commits (from v2.20.1 to master on git.git) to
> > > rebase. No luck.
> >
> > Please, try on this repo: git@github.com:tempesta-tech/mariadb
> >
> > ```
> > git checkout 62a082f573
> > git rebase -p -x /tmp/check.sh ca7fbcea6c4
> > ```
>
> It doesn't reproduce for me.
>
> Usually when we see racy contention on index.lock, the culprit turns out
> to be another unrelated git process refreshing the index. Do you have
> anything else running which might be using "git status" (e.g., magit in
> emacs, vim git integration, etc)?
>

kdevelop which is git-aware. But if git fails on concurrent operation
this is still not good. I would expect it to wait until lock releases
for some time.

I confirm, without kdevelop running it doesn't reproduce.

> -Peff



-- 
All the best,

Aleksey Midenkov
@midenok

Re: Bug: fatal: Unable to create '.../.git/index.lock': File exists.

[Jeff King]

On Wed, May 01, 2019 at 10:15:19AM +0300, Aleksey Midenkov wrote:

> > Usually when we see racy contention on index.lock, the culprit turns out
> > to be another unrelated git process refreshing the index. Do you have
> > anything else running which might be using "git status" (e.g., magit in
> > emacs, vim git integration, etc)?
> 
> kdevelop which is git-aware. But if git fails on concurrent operation
> this is still not good. I would expect it to wait until lock releases
> for some time.

Historically Git does not wait for locks because whoever is holding the
lock is likely to invalidate the changes we're proposing to make by
taking the lock in the first place. We've softened on that a bit in
recent years (e.g., ref updates now retry with a timeout to accommodate
things like reflog pruning), but I don't think the index code does.

If the other entity holding the lock is just updating the stat
information in the index, that's probably OK. If it's actually
manipulating the index, I think we'd have to give more thought about
whether that's safe.

Assuming that kdevelop is just running "git status" in the background,
though, there's an easier solution. If it uses "git --no-optional-locks
status" instead, that will instruct it not to take the index lock at
all.

-Peff

Re: Bug: fatal: Unable to create '.../.git/index.lock': File exists.

[Aleksey Midenkov]

On Wed, May 1, 2019 at 9:36 PM Jeff King <peff@peff.net> wrote:
>
> On Wed, May 01, 2019 at 10:15:19AM +0300, Aleksey Midenkov wrote:
>
> > > Usually when we see racy contention on index.lock, the culprit turns out
> > > to be another unrelated git process refreshing the index. Do you have
> > > anything else running which might be using "git status" (e.g., magit in
> > > emacs, vim git integration, etc)?
> >
> > kdevelop which is git-aware. But if git fails on concurrent operation
> > this is still not good. I would expect it to wait until lock releases
> > for some time.
>
> Historically Git does not wait for locks because whoever is holding the
> lock is likely to invalidate the changes we're proposing to make by
> taking the lock in the first place. We've softened on that a bit in
> recent years (e.g., ref updates now retry with a timeout to accommodate
> things like reflog pruning), but I don't think the index code does.
>
> If the other entity holding the lock is just updating the stat
> information in the index, that's probably OK. If it's actually
> manipulating the index, I think we'd have to give more thought about
> whether that's safe.
>
> Assuming that kdevelop is just running "git status" in the background,
> though, there's an easier solution. If it uses "git --no-optional-locks
> status" instead, that will instruct it not to take the index lock at
> all.

And can we disable optional locks at git configuration level? Because
changing source code of each application that is not aware of this
option is not an easier solution.

>
> -Peff



-- 
All the best,

Aleksey Midenkov
@midenok

Re: Bug: fatal: Unable to create '.../.git/index.lock': File exists.

[Jeff King]

On Thu, May 02, 2019 at 04:45:36PM +0300, Aleksey Midenkov wrote:

> > Assuming that kdevelop is just running "git status" in the background,
> > though, there's an easier solution. If it uses "git --no-optional-locks
> > status" instead, that will instruct it not to take the index lock at
> > all.
> 
> And can we disable optional locks at git configuration level? Because
> changing source code of each application that is not aware of this
> option is not an easier solution.

Since the decision of whether to use the locks is dependent on the
operation being performed, it's an environment variable and not a config
option. You should be able to do:

  GIT_OPTIONAL_LOCKS=0 kdevelop

and any commands run by kdevelop will avoid taking locks when they can
(but for now, the only command which does this is git-status anyway).

-Peff

Re: Bug: fatal: Unable to create '.../.git/index.lock': File exists.

[Duy Nguyen]

On Thu, May 2, 2019 at 10:07 PM Jeff King <peff@peff.net> wrote:
>
> On Thu, May 02, 2019 at 04:45:36PM +0300, Aleksey Midenkov wrote:
>
> > > Assuming that kdevelop is just running "git status" in the background,
> > > though, there's an easier solution. If it uses "git --no-optional-locks
> > > status" instead, that will instruct it not to take the index lock at
> > > all.
> >
> > And can we disable optional locks at git configuration level? Because
> > changing source code of each application that is not aware of this
> > option is not an easier solution.
>
> Since the decision of whether to use the locks is dependent on the
> operation being performed, it's an environment variable and not a config
> option.

And there's also tradeoff for doing it. If git-status will not take
locks, it cannot update the index to save refresh information and
reuse the next time. git-status may become more and more expensive
over time (*). Setting a config variable for this does not sound like
a good idea at all. The same for setting GIT_OPTIONAL_LOCKS=0 in
~/.bashrc to "fix" the problem once and for all.

I might take a stab at the "wait and try to hold the lock again, doing
necessary verification after if needed" idea. It sounds like the right
way to go and we haven't had problems with refs doing the same thing
(have we?).

(*) not entirely true since other commands can also refresh and save.
But in the ideal world when optional locks are used for all optional
updates, it's true.

> You should be able to do:
>
>   GIT_OPTIONAL_LOCKS=0 kdevelop
>
> and any commands run by kdevelop will avoid taking locks when they can
> (but for now, the only command which does this is git-status anyway).
-- 
Duy

Re: Bug: fatal: Unable to create '.../.git/index.lock': File exists.

[Jeff King]

On Thu, May 02, 2019 at 11:38:51PM +0700, Duy Nguyen wrote:

> > Since the decision of whether to use the locks is dependent on the
> > operation being performed, it's an environment variable and not a config
> > option.
> 
> And there's also tradeoff for doing it. If git-status will not take
> locks, it cannot update the index to save refresh information and
> reuse the next time. git-status may become more and more expensive
> over time (*). Setting a config variable for this does not sound like
> a good idea at all. The same for setting GIT_OPTIONAL_LOCKS=0 in
> ~/.bashrc to "fix" the problem once and for all.

Right. I suspect in the long run it might not be _too_ bad to run with
such a setting, because any non-read operations would eventually refresh
the index (and as you note even many read-only operations like porcelain
git-diff unconditionally refresh for now).

But I agree it's not really the direction we want to go.

> I might take a stab at the "wait and try to hold the lock again, doing
> necessary verification after if needed" idea. It sounds like the right
> way to go and we haven't had problems with refs doing the same thing
> (have we?).

No, but it's a bit easier with refs because the locking is just
atomically checking the lease. I.e., after taking the lock we still say
"we expected the ref to be at oid XYZ, is it still there?". What's the
equivalent for an index operation?

I think it is more common with the index to take the lock, then while
holding it read it in fresh (possibly dumping old results), manipulate
the result, and then write it out. For callers which make sure to
get a fresh view _after_ taking the lock, they should be OK if taking
the lock is delayed.

I guess arguably any callers that aren't that careful are already
broken, since it is a race; any delay-and-retry _could_ have happened as
"we were too slow to see the initial lock".

-Peff

Re: Bug: fatal: Unable to create '.../.git/index.lock': File exists.

[Duy Nguyen]

On Thu, May 2, 2019 at 11:58 PM Jeff King <peff@peff.net> wrote:
> > I might take a stab at the "wait and try to hold the lock again, doing
> > necessary verification after if needed" idea. It sounds like the right
> > way to go and we haven't had problems with refs doing the same thing
> > (have we?).
>
> No, but it's a bit easier with refs because the locking is just
> atomically checking the lease. I.e., after taking the lock we still say
> "we expected the ref to be at oid XYZ, is it still there?". What's the
> equivalent for an index operation?

That's something for me to find out :)

> I think it is more common with the index to take the lock, then while
> holding it read it in fresh (possibly dumping old results), manipulate
> the result, and then write it out. For callers which make sure to
> get a fresh view _after_ taking the lock, they should be OK if taking
> the lock is delayed.
>
> I guess arguably any callers that aren't that careful are already
> broken, since it is a race; any delay-and-retry _could_ have happened as
> "we were too slow to see the initial lock".

I have a feeling that most operations read the index unlocked,
manipulate and only lock before writing things out. So yeah it's
probably already racy.

We could use the trailing SHA-1 to determine if the index has not
changed since last time, but then refresh-only updates would be
considered valuable while it's not. Full index comparison is way too
expensive (at least with giant repos) to even consider. I think I
start to see why nobody has done this...
-- 
Duy

Re: Bug: fatal: Unable to create '.../.git/index.lock': File exists.

[Duy Nguyen]

On Thu, May 2, 2019 at 11:58 PM Jeff King <peff@peff.net> wrote:
> > I might take a stab at the "wait and try to hold the lock again, doing
> > necessary verification after if needed" idea. It sounds like the right
> > way to go and we haven't had problems with refs doing the same thing
> > (have we?).
>
> No, but it's a bit easier with refs because the locking is just
> atomically checking the lease. I.e., after taking the lock we still say
> "we expected the ref to be at oid XYZ, is it still there?". What's the
> equivalent for an index operation?

We could add a second hash that only covers the valuable parts (e.g.
paths, stage index, SHA-1 and probably some ce_flags). This should be
enough to verify if the index is still the same as before (stat info
does not count, the same for other cache data like untracked cache,
cache-tree...).

This will add some overhead of course because we hash more, especially
on large index files. So it will be optional. The hash is stored in an
extension. If you run things in parallel and want this, enable it. If
the extension is not present in the first place, we don't attempt to
wait and retry or anything.
-- 
Duy

Re: Bug: fatal: Unable to create '.../.git/index.lock': File exists.

[Johannes Schindelin]

Hi Duy,

On Fri, 3 May 2019, Duy Nguyen wrote:

> I have a feeling that most operations read the index unlocked,
> manipulate and only lock before writing things out. So yeah it's
> probably already racy.

IIRC there is a check for that, so it is not actually racy ;-)

Ciao,
Johannes

Re: Bug: fatal: Unable to create '.../.git/index.lock': File exists.

[Duy Nguyen]

On Fri, May 3, 2019 at 4:47 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Duy,
>
> On Fri, 3 May 2019, Duy Nguyen wrote:
>
> > I have a feeling that most operations read the index unlocked,
> > manipulate and only lock before writing things out. So yeah it's
> > probably already racy.
>
> IIRC there is a check for that, so it is not actually racy ;-)

Yeah the update_if_able(), used exclusively for refreshing index. My
feeling was wrong actually. Looking around a bit, I think we do take
the lock, re-read the index, do things, then write.

There may be racy spots still. Looking quickly through some
write_locked_index callsites, difftool.c and checkout-index.c may
leave a gap between loading the index and locking it. Or
refresh_and_write_cache() and a couple others in am.c do not look
exactly race-free.

We probably should provide an API where locking requires re-reading
the index. The version without re-reading has a big fat warning about
danger and stuff.

In any case, i'm getting off topic. I'll stop here.
-- 
Duy