Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make nix copy parallel again #6612

Merged
merged 11 commits into from
Aug 24, 2022
Merged

Make nix copy parallel again #6612

merged 11 commits into from
Aug 24, 2022

Conversation

thufschmitt
Copy link
Member

@thufschmitt thufschmitt commented Jun 3, 2022

nix copy used to do stuff in parallel. This got removed in #5048 (to allow batching the copies in the ssh store case), but both can actually be kept in parallel with a little effort.

This is currently incomplete as it

  • Disables the batch ssh copy
  • Removes the ability to copy CA paths from one store prefix to another (I'm curious what the use-case for this is)

cc @ConnorBaker

@thufschmitt
Copy link
Member Author

This is currently incomplete as it...

Also the code is a bit of a mess right now

Théophane Hufschmitt added 3 commits June 8, 2022 14:03
Bring back the possibility to copy CA paths with no reference (like the
outputs of FO derivations or stuff imported at eval time) between stores
that have a different prefix.
@ConnorBaker
Copy link
Contributor

Thank you so much for doing this @thufschmitt! This change has been invaluable in helping me quickly copy things to a binary cache I set up for CI/CD.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-32/19865/1

@edolstra edolstra added this to the nix-2.10 milestone Jul 1, 2022
@edolstra edolstra modified the milestones: nix-2.10, nix-2.11 Jul 11, 2022
Like the old implem did (and like you'd want it to be anyways)
@thufschmitt thufschmitt marked this pull request as ready for review July 20, 2022 08:07
@thufschmitt thufschmitt requested a review from edolstra July 20, 2022 08:07
This hang for some reason didn't trigger in the Nix build, but did
running 'make installcheck' interactively. What happened:

* Store::addMultipleToStore() calls a SinkToSource object to copy a
  path, which in turn calls LegacySSHStore::narFromPath(), which
  acquires a connection.

* The SinkToSource object is not destroyed after the last bytes has
  been read, so the coroutine's stack is still alive and its
  destructors are not run. So the connection is not released.

* Then when the next path is copied, because max-connections = 1,
  LegacySSHStore::narFromPath() hangs forever waiting for a connection
  to be released.

The fix is to make sure that the source object is destroyed when we're
done with it.
@edolstra
Copy link
Member

Fixed a subtle hang in f0358ed.

@Ericson2314
Copy link
Member

Interesting bug! So this is something that source's own destructor doesn't do? Is that intended?

@edolstra
Copy link
Member

The problem is that source's destructor doesn't run until the unique_ptr is released.

@Ericson2314
Copy link
Member

Oh thanks I see now.

I think it might be better to move the unique pointer out of the map (leaving behind a null one, I suppose). And then we get the right destructor call by default.

@edolstra edolstra enabled auto-merge August 24, 2022 12:11
@edolstra edolstra disabled auto-merge August 24, 2022 12:46
@edolstra
Copy link
Member

@Ericson2314 You're right, in fact moving it was necessary to get it to build on macOS.

@edolstra edolstra enabled auto-merge August 24, 2022 12:53
@edolstra edolstra merged commit 04e74f7 into master Aug 24, 2022
@edolstra edolstra deleted the parallel-nix-copy branch August 24, 2022 13:31
Minion3665 pushed a commit to Minion3665/nix that referenced this pull request Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants