Commit graph

16336 commits

Author SHA1 Message Date
jade a0fb52c0af Fix std::terminate call in thread pool
So we received a report that the thread pool crashed due to an
Interrupted exception.

Relevant log tail:

copying path '/nix/store/0kal2k73inviikxv9f1ciaj39lkl9a87-etc-os-release' to 'ssh://192.168.0.27'...
Lix crashed. This is a bug. We would appreciate if you report it along with what caused it at https://git.lix.systems/lix-project/lix/issues with the following information included:

error (ignored): error: interrupted by the user
Exception: nix::Interrupted: error: interrupted by the user

Relevant stack trace:

 4# __cxa_rethrow in /nix/store/22nxhmsfcv2q2rpkmfvzwg2w5z1l231z-gcc-13.3.0-lib/lib/libstdc++.so.6
 5# nix::ignoreExceptionExceptInterrupt(nix::Verbosity) in /nix/store/ghxr2ykqc3rrfcy8rzdys0rzx9ah5fqj-lix-2.92.0-dev-pre20241005-ed9b7f4/lib/liblixutil.so
 6# nix::ThreadPool::doWork(bool) in /nix/store/ghxr2ykqc3rrfcy8rzdys0rzx9ah5fqj-lix-2.92.0-dev-pre20241005-ed9b7f4/lib/liblixutil.so
 7# 0x00007FA7A00E86D3 in /nix/store/22nxhmsfcv2q2rpkmfvzwg2w5z1l231z-gcc-13.3.0-lib/lib/libstdc++.so.6
 8# 0x00007FA79FE99A42 in /nix/store/3dyw8dzj9ab4m8hv5dpyx7zii8d0w6fi-glibc-2.39-52/lib/libc.so.6
 9# 0x00007FA79FF1905C in /nix/store/3dyw8dzj9ab4m8hv5dpyx7zii8d0w6fi-glibc-2.39-52/lib/libc.so.6

Notably, this is *not* in the main thread, so this implies that the
thread didn't get joined properly before their destructors got called.
That, in turn, should have only possibly happened because join() threw
on a previous iteration of the loop joining threads, I think. Or if it
threw while in the ThreadPool destructor. Either way we had better stop
letting Interrupted fall out of our child threads!

If:
- Interrupted was thrown inside the action in the main thread: it would
  have fallen out of doWork if state->exception was already set and got
  caught by ThreadPool::process, calling shutdown() and the join loop
  which would crash the process entirely.
- Interrupted was thrown inside the action on a secondary thread: it
  would have been caught and put into the exception field and then
  possibly rethrown to fall out of the thread (since it was previously
  ignoreExceptionExceptInterrupt).

The one possible hole in this hypothesis is that there is an "error
(ignored)" line in there implying that at least one Interrupted got
eaten by an ignoreExceptionInDestructor. It's also unclear whether this
got reordered because of stderr buffering.

Fixes: #542
Change-Id: I322cf050da660af78f5cb0e08ec6e6d27d09ac76
2024-10-09 15:38:40 -07:00
jade 9865ebaaa6 Merge "Remove static initializers for RegisterLegacyCommand" into main 2024-10-09 20:37:58 +00:00
jade 7f7a38f278 Merge changes Ib27cb43d,I03687b8b into main
* changes:
  testsuite: override NIX_CONF_DIR and NIX_USER_CONF_FILES
  Remove some outdated `make test` invocation suggestions
2024-10-09 20:37:16 +00:00
Rebecca Turner 0012887310 Merge "Add release note for CTRL-C improvements" into main 2024-10-08 22:15:56 +00:00
Lulu 4ea8c9d643 Set c++ version to c++23
I followed @pennae's advice and moved the constructor definition of
`AttrName` from the header file `nixexpr.hh` to `nixexpr.cc`.

Change-Id: I733f56c25635b366b11ba332ccec38dd7444e793
2024-10-08 20:05:28 +02:00
Lulu 43e79f4434 Fix gcc warning -Wmissing-field-initializers
The approach that was taken here was to add default values to the type
definitions rather than specify them whenever they are missing.

Now the only remaining warning is '-Wunused-parameter' which @jade said
is usually counterproductive and that we can just disable it:
#456 (comment)

So this change adds the flags '-Wall', '-Wextra' and
'-Wno-unused-parameter', so that all warnings are enabled except for
'-Wunused-parameter'.

Change-Id: Ic223a964d67ab429e8da804c0721ba5e25d53012
2024-10-08 01:44:38 +00:00
Lulu 299813f324 Merge "Avoid calling memcpy when len == 0 in filetransfer.cc" into main 2024-10-08 01:41:41 +00:00
Lulu d6e1b11d3e Fix gcc warning -Wsign-compare
Add the compile flag '-Wsign-compare' and adapt the code to fix all
cases of this warning.

Change-Id: I26b08fa5a03e4ac294daf697d32cf9140d84350d
2024-10-08 01:32:12 +02:00
Lulu 51a5025913 Avoid calling memcpy when len == 0 in filetransfer.cc
There was a bug report about a potential call to `memcpy` with a null
pointer which is not reproducible:
#492

This occurred in `src/libstore/filetransfer.cc` in `InnerSource::read`.

To ensure that this doesn't happen, an early return is added before
calling `memcpy` if the length of the data to be copied is 0.

This change also adds a test that ensures that when `InnerSource::read`
is called with an empty file, it throws an `EndOfFile` exception.

Change-Id: Ia18149bee9a3488576c864f28475a3a0c9eadfbb
2024-10-08 01:26:30 +02:00
eldritch horrors ed9b7f4f84 libstore: remove Worker::{childStarted, goalFinished}
these two functions are now nearly trivial and much better inline into
makeGoalCommon. keeping them separate also separates information about
goal completion flows and how failure information ends up in `Worker`.

Change-Id: I6af86996e4a2346583371186595e3013c88fb082
2024-10-05 21:19:51 +00:00
eldritch horrors 649d8cd08f libstore: remove Worker::removeGoal
we can use our newfound powers of Goal::work Is A Real Promise to remove
completed goals from continuation promises. apart from being much easier
to follow it's also a lot more efficient because we have the iterator to
the item we are trying to remove, skipping a linear search of the cache.

Change-Id: Ie0190d051c5f4b81304d98db478348b20c209df5
2024-10-05 21:19:51 +00:00
eldritch horrors 9adf6f4568 libstore: remove Goal::notify
Goal::work() is a fully usable promise that does not rely on the worker
to report completion conditions. as such we no longer need the `notify`
field that enabled this interplay. we do have to clear goal caches when
destroying the worker though, otherwise goal promises may (incorrectly)
keep goals alive due to strong shared pointers created by childStarted.

Change-Id: Ie607209aafec064dbdf3464fe207d70ba9ee158a
2024-10-05 21:19:51 +00:00
eldritch horrors 03cbc0ecb9 libstore: move Goal::ex to WorkResult
yet another duplicated field. it's the last one though.

Change-Id: I352df8d306794d262d8c9066f3be78acd40e82cf
2024-10-05 21:19:51 +00:00
eldritch horrors 1caf2afb1d libstore: move Goal::buildResult to WorkResult
derivation goals still hold a BuildResult member variable since parts of
these results of accumulated in different places, but the Goal class now
no longer has such a field. substitution goals don't need it at all, and
derivation goals should also be refactored to not drop their buildResult

Change-Id: Ic6d3d471cdbe790a6e09a43445e25bedec6ed446
2024-10-05 20:53:39 +00:00
eldritch horrors 7ff60b7445 libstore: move Goal::exitCode to WorkResult
the field is simply duplicated between the two, and now that we can
return WorkResults from Worker::run we no longer need both of them.

Change-Id: I82fc47d050b39b7bb7d1656445630d271f6c9830
2024-10-05 20:17:20 +00:00
eldritch horrors fc6291e46d libstore: return goal results from Worker::run()
this will be needed to move all interesting result fields out of Goal
proper and into WorkResult. once that is done we can treat goals as a
totally internal construct of the worker mechanism, which also allows
us to fully stop exposing unclear intermediate state to Worker users.

Change-Id: I98d7778a4b5b2590b7b070bdfc164a22a0ef7190
2024-10-05 20:12:13 +00:00
eldritch horrors 40f154c0ed libstore: remove Worker::topGoals
since we now propagate goal exceptions properly we no longer need to
check topGoals for a reason to abort early. any early abort reasons,
whether by exception or a clean top goal failure, can now be handled
by inspecting the goal result in the main loop. this greatly reduces
goal-to-goal interactions that do not happen at the main loop level.

since the underscore-free name is now available for use as variables
we'll migrate to that where we currently use `_topGoals` for locals.

Change-Id: I5727c5ea7799647c0a69ab76975b1a03a6558aa6
2024-10-05 19:53:30 +00:00
eldritch horrors f389a54079 libstore: propagate goal exceptions using promises
drop childException since it's no longer needed. also makes
waitForInput, childFinished, and childTerminated redundant.

Change-Id: I05d88ffd323c5b5c909ac21056162f69ffb0eb9f
2024-10-05 19:44:47 +00:00
eldritch horrors 7ef4466018 libstore: have goals promise WorkResults, not void
Change-Id: Idd218ec1572eda84dc47accc0dcd8a954d36f098
2024-10-05 19:06:59 +00:00
eldritch horrors a9f2aab226 libstore: extract Worker::goalFinished specifics
there's no reason to have the worker set information on goals that the
goals themselves return from their entry point. doing this in the goal
`work()` function is much cleaner, and a prerequisite to removing more
implicit strong shared references to goals that are currently running.

Change-Id: Ibb3e953ab8482a6a21ce2ed659d5023a991e7923
2024-10-05 19:06:59 +00:00
eldritch horrors 99edc2ae38 libstore: check for interrupts in parallel promise
this simplifies the worker loop, and lets us remove it entirely later.
note that ideally only one promise waiting for interrupts should exist
in the entire system. not one per event loop, one per *process*. extra
interrupt waiters make interrupt response nondeterministic and as such
aren't great for user experience. if anything wants to react to aborts
caused by explicit interruptions, or anything else, those things would
be much better served using RAII guards such as Finally (or KJ_DEFER).

Change-Id: I41d035ff40172d536e098153c7375b0972110d51
2024-10-05 19:06:59 +00:00
eldritch horrors 896a123605 libstore: remove Goal::StillAlive
this was a triumph. i'm making a note here: huge success. it's hard to
overstate my satisfaction! i'm not even angry. i'm being so sincere ri

actually, no. we *are* angry. this was one dumbass odyssey. nobody has
asked for this. but not doing it would have locked us into old, broken
protocols forever or (possibly worse) forced us to write our own async
framework building on the old did-you-mean-continuations in Worker. if
we had done that we'd be locked into ever more, and ever more complex,
manual state management all over the place. this just could not stand.

Change-Id: I43a6de1035febff59d2eff83be9ad52af4659871
2024-10-05 18:21:02 +00:00
Rebecca Turner 0d484aa498
Add release note for CTRL-C improvements
I'm very excited for cl/2016, so others will probably be excited also!
Let's add a release note.

Change-Id: Ic84a4444241aafce4cb6d5a6d1dddb47e7a7dd7b
2024-10-05 10:40:51 -07:00
Rebecca Turner 86b213e632 Merge "Split ignoreException to avoid suppressing CTRL-C" into main 2024-10-05 17:33:00 +00:00
eldritch horrors a3dd07535c fix build test error count checks
with async runtime scheduling we can no longer guarantee exact error
counts for builds that do not set keepGoing. the old behavior can be
recovered with a number of hacks that affect scheduling, but none of
those are very easy to follow now advisable. exact error counts will
like not be needed for almost all uses except tests, and *those* had
better check the actual messages rather than how many they got. more
messages can even help to avoid unnecessary rebuilds for most users.

Change-Id: I1c9aa7a401227dcaf2e19975b8cb83c5d4f85d64
2024-10-05 16:21:19 +00:00
alois31 5df2cccc49
doc: install the HTML manual again
In 0e6b3435a1, installation of the HTML manual
was accidentally dropped: setting install_dir on a custom_target only sets the
directory where something is going to be installed if it is installed at all,
but does not itself trigger installation. The latter has to be explicitly
requested, which is just what we do here to get the manual back.

Change-Id: Iff8b791de7e7cb4c8d747c2a9b1154b5fcc32fe0
2024-10-05 10:49:34 +02:00
jade 345e3d068a testsuite: override NIX_CONF_DIR and NIX_USER_CONF_FILES
The test suite can load the global configuration files under certain
circumstances, and, though we would really rather it didn't ever do that
at all, we should at least break the mechanism.

Fixes: #474
Change-Id: Ib27cb43dd5dfaa70ac491c395b5ba308fd7bd289
2024-10-04 19:17:08 -07:00
jade 19edaed81b Remove some outdated make test invocation suggestions
These should be meson.

Change-Id: I03687b8b03f50fb1684e7ffcd487be855052d6c2
2024-10-04 18:55:52 -07:00
eldritch horrors 5b1715e633 libstore: forbid addWantedGoals when finished
due to event loop scheduling behavior it's possible for a derivation
goal to fully finish (having seen all paths it was asked to create),
but to not notify the worker of this in time to prevent another goal
asking the recently-finished goal for more outputs. if this happened
the finished goal would ignore the request for more outputs since it
considered itself fully done, and the delayed result reporting would
cause the requesting goal to assume its request had been honored. if
the requested goal had finished *properly* the worker would recreate
it instead of asking for more outputs, and this would succeed. it is
thus safe to always recreate goals once they are done, so we now do.

Change-Id: Ifedd69ca153372c623abe9a9b49cd1523588814f
2024-10-04 17:49:57 +00:00
Rebecca Turner 0b29859cfe Merge "editorconfig: Add meson.build" into main 2024-10-04 16:36:20 +00:00
Olivia Crain 1bfc37fea5 Merge "internal-api-docs: allow Doxygen to build regardless of workdir" into main 2024-10-04 09:59:01 +00:00
Olivia Crain 8f300fbd82 Merge "build: let meson add compiler flags for libstdc++ assertions" into main 2024-10-04 09:58:32 +00:00
Rebecca Turner 36073781fb
editorconfig: Add meson.build
Change-Id: Ibb59ddc21f5d3ef7fb4c900e3413e426c201334d
2024-10-01 16:09:47 -07:00
Rebecca Turner b63d4a0c62
Remove static initializers for RegisterLegacyCommand
This moves the "legacy"/"nix2" commands under a new `src/legacy/`
directory, instead of being scattered around in a bunch of different
directories.

A new `liblegacy` build target is defined, and the `nix` binary is
linked against it.

Then, `RegisterLegacyCommand` is replaced with `LegacyCommand::add`
calls in functions like `registerNixCollectGarbage()`. These
registration functions are called explicitly in `src/nix/main.cc`.

See: #359

Change-Id: Id450ffc3f793374907599cfcc121863b792aac1a
2024-10-01 16:08:58 -07:00
Robert Hensing ee0c195eba
Split ignoreException to avoid suppressing CTRL-C
This splits `ignoreException` into `ignoreExceptionExceptInterrupt`
(which ignores all exceptions except `Interrupt`, which indicates a
SIGINT/CTRL-C) and `ignoreExceptionInDestructor` (which ignores all
exceptions, so that destructors do not throw exceptions).

This prevents many cases where Nix ignores CTRL-C entirely.
See: https://github.com/NixOS/nix/issues/7245

Upstream-PR: https://github.com/NixOS/nix/pull/11618
Change-Id: Ie7d2467eedbe840d1b9fa2e88a4e88e4ab26a87b
2024-10-01 15:49:56 -07:00
eldritch horrors 7752927660 libstore: turn DerivationGoal::work into *one* promise
Change-Id: Ic2f7bc2bd6a1879ad614e4be81a7214f64eb0e85
2024-10-01 11:55:47 +00:00
eldritch horrors 3edc272341 libstore: turn DrvOutputSubstitutionGoal::work into *one* promise
Change-Id: I2d4dcedff0a278d2d8f3d264a9186dfb399275e2
2024-10-01 11:55:42 +00:00
eldritch horrors 9b05636937 libstore: make PathSubstitutionGoal::work *one* promise
Change-Id: I38cfe8c7059251b581f1013c4213804f36b985ea
2024-10-01 11:55:36 +00:00
eldritch horrors 9889c79fe3 libstore: turn Worker::updateStatistics into a promise
we'll now loop to update displayed statistics, and use this loop to
limit the update rate to 50 times per second. we could have updated
much more frequently before this (once per iteration of `runImpl`),
much faster than would ever be useful in practice. aggressive stats
updates can even impede progress due to terminal or network delays.

Change-Id: Ifba755a2569f73c919b1fbb06a142c0951395d6d
2024-10-01 11:55:29 +00:00
eldritch horrors 732de75f67 libstore: remove Worker::wakeUp()
Worker::run() is now entirely based on the kj event loop and promises,
so we need not handle awakeness of goals manually any more. every goal
can instead, once it has finished a partial work call, defer itself to
being called again in the next iteration of the loop. same end effect.

Change-Id: I320eee2fa60bcebaabd74d1323fa96d1402c1d15
2024-10-01 13:55:03 +02:00
eldritch horrors d5db0b1abc libstore: turn periodic gc attempt into a promise
notably we will check whether we want to do GC at all only once during
startup, and we'll only attempt GC every ten seconds rather than every
time a goal has finished a partial work call. this shouldn't cause any
problems in practice since relying on auto-gc is not deterministic and
stores in which builds can fill all remaining free space in merely ten
seconds are severely troubled even when gargage collection runs a lot.

Change-Id: I1175a56bf7f4e531f8be90157ad88750ff2ddec4
2024-10-01 11:36:45 +00:00
eldritch horrors b0c7c1ec66 libstore: turn Worker::run() main loop into a promise
Change-Id: Ib112ea9a3e67d5cb3d7d0ded30bbd25c96262470
2024-10-01 11:36:45 +00:00
eldritch horrors d31310bf59 libstore: turn waitForInput into a promise
Change-Id: I8355d8d3f6c43a812990c1912b048e5735b07f7b
2024-10-01 11:36:45 +00:00
raito 8e05cc1e6c Revert "libstore: remove worker removeGoal"
Revert submission 1946

Reason for revert: regression in building (found via bisection)

Reported by users:
> error: path '/nix/store/04ca5xwvasz6s3jg0k7njz6rzi0d225w-jq-1.7.1-dev' does not exist in the store

Reverted changes: /q/submissionid:1946

Change-Id: I6f1a4b2f7d7ef5ca430e477fc32bca62fd97036b
2024-10-01 11:07:57 +00:00
Jonas Chevalier a16ceb9411 Merge "fix(nix fmt): remove the default "." argument" into main 2024-09-30 16:10:32 +00:00
eldritch horrors aa33c34c9b libstore: merge ContinueImmediately and StillAlive
nothing needs to signal being still active but not actively pollable,
only that immediate polling for the next goal work phase is in order.

Change-Id: Ia43c1015e94ba4f5f6b9cb92943da608c4a01555
2024-09-29 15:29:56 +00:00
eldritch horrors ccd2862666 libstore: remove worker removeGoal
this was immensely inefficient on large caches, as can exist when many
derivations are buildable simultaneously. since we have smart pointers
to goals we can do cache maintenance in goal deleters instead, and use
the exact iterators instead of doing a linear search. this *does* rely
on goals being deleted to remove them from the cache, which isn't true
for toplevel goals. those would have previously been removed when done
in all cases, removing the cache entry when keep-going is set. this is
arguably incorrect since it might result in those goals being retried,
although that could only happen with dynamic derivations or the likes.
(luckily dynamic derivations not complete enough to allow this at all)

Change-Id: I8e750b868393588c33e4829333d370f2c509ce99
2024-09-29 15:29:56 +00:00
eldritch horrors 47ddd11933 libstore: extract a real makeGoalCommon
makeDerivationGoalCommon had the right idea, but it didn't quite go far
enough. let's do the rest and remove the remaining factory duplication.

Change-Id: I1fe32446bdfb501e81df56226fd962f85720725b
2024-09-29 15:07:30 +00:00
eldritch horrors 7f4f86795c libstore: remove Goal::key
this was a debugging aid from day one that should not have any impact on
build semantics, and if it *does* have an impact on build semantics then
build semantics are seriously broken. keeping the order imposed by these
keys will be impossible once we let a real event loop schedule our jobs.

Change-Id: I5c313324e1f213ab6453d82f41ae5e59de809a5b
2024-09-29 14:29:14 +00:00
eldritch horrors a5240b23ab libstore: make non-cache goal pointers strong
without circular references we do not need weak goal pointers except for
caches, which should not prevent goal destructors running. caches though
cannot create circular references even when they keep strong references.
if we removed goals from caches when their work() is fully finished, not
when their destructors are run, we could keep strong pointers in caches.
since we do not gain much from this we keep those pointers weak for now.

Change-Id: I1d4a6850ff5e264443c90eb4531da89f5e97a3a0
2024-09-29 14:29:14 +00:00