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

libstore: always use the socket if it exists #4263

Closed
wants to merge 1 commit into from

Conversation

zimbatm
Copy link
Member

@zimbatm zimbatm commented Nov 16, 2020

This change modifies the behaviour of Nix to always communicate with the
daemon, even if the store is writable to that user.

Bypassing the daemon had a number of issues. Mainly, the client
behaviour is not always consistent between users leading to confusion.
The max-jobs counter is not shared between various instances of the Nix
client. The clients fight between each-other to hold the lock on the
sqlite database.

By making the access consistent, it opens the road for the daemon to
permanently hold the lock on the sqlite database, and get rid of the
"sqlite is busy" type of errors.

This change modifies the behaviour of Nix to always communicate with the
daemon, even if the store is writable to that user.

Bypassing the daemon had a number of issues. Mainly, the client
behaviour is not always consistent between users leading to confusion.
The max-jobs counter is not shared between various instances of the Nix
client. The clients fight between each-other to hold the lock on the
sqlite database.

By making the access consistent, it opens the road for the daemon to
permanently hold the lock on the sqlite database, and get rid of the
"sqlite is busy" type of errors.
@edolstra
Copy link
Member

This makes Nix less efficient when running as root, by requiring an unnecessary redirection through the daemon.

By making the access consistent, it opens the road for the daemon to permanently hold the lock on the sqlite database, and get rid of the "sqlite is busy" type of errors.

Not until we have a multi-threaded, non-forking daemon...

@zimbatm
Copy link
Member Author

zimbatm commented Nov 16, 2020

This makes Nix less efficient when running as root

Do you use sudo to improve performance during a build? On my systems, building as a user is the common case so it's not really an optimization I am benefiting from. It's important to remove as many branches as possible in the code.

@edolstra
Copy link
Member

No, but Hydra connects to most build slaves as root.

@zimbatm
Copy link
Member Author

zimbatm commented Nov 17, 2020

This doesn't feel right. Maybe the daemon could be disabled on those machines but that doesn't strike me as a good solution either.

Would you be willing to re-consider that decision if the daemon overhead was reduced sufficiently?

@andir
Copy link
Member

andir commented Nov 17, 2020 via email

@zimbatm
Copy link
Member Author

zimbatm commented Nov 17, 2020

I guess the next step would be to write some sort of benchmarks then.

@7c6f434c
Copy link
Member

Having an option to run something one time with a different NIX_CONF_DIR and nix.conf without messing with the main daemon is what I find a feature, not a bug…

I'd assume that for nix-instantiate the context-switching of the daemon use is more important than how the data is copied?

@edolstra
Copy link
Member

The overhead is having to start another process and send all operations to it via RPC.

What's the actual problem here? I'm not really aware of issues caused by not going through the daemon. In particular

The max-jobs counter is not shared between various instances of the Nix
client. The clients fight between each-other to hold the lock on the
sqlite database.

is not solved by this patch because the daemon worker processes don't share max-jobs or SQLite locks either.

@stale
Copy link

stale bot commented Jun 2, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the stale label Jun 2, 2021
@stale
Copy link

stale bot commented Jun 19, 2022

I closed this issue due to inactivity. → More info

@stale stale bot closed this Jun 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants