Unlock output paths when a derivation is already built #6469
Conversation
| @@ -627,6 +627,7 @@ void DerivationGoal::tryToBuild() | |||
| if (buildMode != bmCheck && allValid) { | |||
| debug("skipping build of derivation '%s', someone beat us to it", worker.store.printStorePath(drvPath)); | |||
| outputLocks.setDeletion(true); | |||
| outputLocks.unlock(); | |||
There was a problem hiding this comment.
It's possible this might be better placed in done, or perhaps buildDone could be used (I'm not too familiar with the code).
I don't think this is the case in general. |
|
Goals are only destructed at the end of |
Yes that matches the behaviour we are seeing. We have a top-level goal with a fairly 'wide' graph of dependencies underneath it. We see already-built goals in the middle of the graph hang around for some time while their siblings are being built, sometimes long enough to cause deadlock with other nix build processes building a similar dependency graph.
I took a look through the other cases and I couldn't immediately see any others that actually take the locks, although I may well have missed something. It looks like the other parts that might take locks (e.g. repairing) create new derivation goals which should go through the same code paths. |
|
We've been running this patch in our production Nix (2.3) builds for over a week now and have observed no deadlocks or other problems, so I'd advocate merging it. |
|
I've also seen deadlocks, with nix-2.7.0. I wasn't able to reproduce the issue outside of the production env, but hoping it's the same issue and that this fixes it. |
|
@edolstra any way we could get this merged for a future Nix release? |
gbpdt
changed the title
fricklerhandwerk
added
store
|
Triaged in Nix team meeting: @gbpdt please move the Side notes:
|
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2023-04-28-nix-team-meeting-minutes-50/27698/1 |
github-actions
bot
removed
the
store
gbpdt
force-pushed
the
fix/skipped_build_locking
branch
from
8269064
to
23da2cd
Compare
Without this change, nix build processes will not drop the locks for derivation goals which have already been built by another process when the current process gets round to building them. This means the locks are held until the process terminates. If there are other nix build processes in a similar state, they will also try to acquire the same locks when they try to build the same derivation, and so will wait until the lock holder terminates (which might be a very long time if it has a lot to build). In some pathological cases, those processes might be holding their own locks on other derivations due to the same issue, and this can lead to deadlock. Resolves NixOS#6468
gbpdt
force-pushed
the
fix/skipped_build_locking
branch
from
23da2cd
to
82ddb13
Compare
Apologies @fricklerhandwerk, I missed this, thanks for taking a look! I've made the change (just moving to |
Unlock output paths when a derivation is already built (cherry picked from commit 7ba4e07) Change-Id: I9de077679290d5141a610ac43d99d3a43acff87c
Without this change, nix build processes will not drop the locks for derivation goals
which have already been built by another process when the current process gets
round to building them. This means the locks are held until the process
terminates.
If there are other nix build processes in a similar state, they will also try to
acquire the same locks when they try to build the same derivation, and so will
wait until the lock holder terminates (which might be a very long time if it has
a lot to build). In some pathological cases, those processes might be holding
their own locks on other derivations due to the same issue, and this can lead to
deadlock.
Resolves #6468