Fix piping nix store ls output on macOS
`writeLogsToStderr` uses a static mutex in order to prevent log output from being interleaved. On macOS, it was possible for a logger in a non-main thread to call this function after the static mutex was destructed, leading to strange errors. Deliberately leaking the mutex prevents the destructor from being called, fixing the issue. Closes #702 Co-Authored-By: Jade Lovelace <jadel@mercury.com> Co-Authored-By: eldritch horrors <pennae@lix.systems> Change-Id: I14c80134cc493972752fad56b7f15fad8e4d5a5b
This commit is contained in:
committed by
Jade Lovelace
parent
99bc6867e8
commit
20fea96996
+14
-2
@@ -4,6 +4,7 @@
|
||||
#include "lix/libutil/config.hh"
|
||||
#include "lix/libutil/position.hh"
|
||||
#include "lix/libutil/terminal.hh"
|
||||
#include "manually-drop.hh"
|
||||
|
||||
#include <algorithm>
|
||||
#include <atomic>
|
||||
@@ -349,7 +350,18 @@ Activity::~Activity()
|
||||
|
||||
void writeLogsToStderr(std::string_view s)
|
||||
{
|
||||
static std::mutex lock;
|
||||
// NOTE: If this lock is a regular static item (and not something
|
||||
// indestructible), then it will be destructed when Nix shuts down. When
|
||||
// other threads are running, it becomes possible for a static to be
|
||||
// destructed before Nix ends, leading to errors.
|
||||
//
|
||||
// Therefore, we use a wrapper type to block it ever getting destroyed.
|
||||
//
|
||||
// TODO: Audit other statics for this issue?
|
||||
//
|
||||
// See: https://git.lix.systems/lix-project/lix/issues/702
|
||||
// See: https://stackoverflow.com/a/27671727/5719760
|
||||
static ManuallyDrop<std::mutex> lock {std::in_place_t{}};
|
||||
|
||||
// make sure only one thread uses this function at any given time.
|
||||
// multiple concurrent threads can have deleterious effects on log
|
||||
@@ -357,7 +369,7 @@ void writeLogsToStderr(std::string_view s)
|
||||
// on top of a SimpleLogger which is itself not thread-safe. every
|
||||
// Logger instance should be thread-safe in an ideal world, but we
|
||||
// cannot really enforce that on a per-logger level at this point.
|
||||
std::unique_lock _lock(lock);
|
||||
std::unique_lock _lock(*lock);
|
||||
try {
|
||||
writeFull(STDERR_FILENO, s, false);
|
||||
} catch (SysError & e) {
|
||||
|
||||
@@ -0,0 +1,89 @@
|
||||
#pragma once
|
||||
/// @file Manually destroy a value; suppresses automatic destruction
|
||||
#include <atomic>
|
||||
#include <cassert>
|
||||
#include <utility>
|
||||
|
||||
namespace nix {
|
||||
|
||||
/** Analogous to Rust's ManuallyDrop structure. Only is destroyed when you call destroy(). */
|
||||
template<typename T>
|
||||
class ManuallyDrop
|
||||
{
|
||||
alignas(alignof(T)) char data[sizeof(T)];
|
||||
// We have to use atomic<bool> here since once_flag reattempts if the
|
||||
// callee throws (double destruction? yikes)
|
||||
std::atomic<bool> destroyed = false;
|
||||
|
||||
public:
|
||||
explicit ManuallyDrop(T && t)
|
||||
{
|
||||
::new (data) T(std::move(t));
|
||||
}
|
||||
|
||||
/** Construct a ManuallyDrop in-place */
|
||||
template<typename... Arg>
|
||||
ManuallyDrop(std::in_place_t, Arg &&... args)
|
||||
{
|
||||
::new (data) T(std::forward<Arg>(args)...);
|
||||
}
|
||||
|
||||
ManuallyDrop(ManuallyDrop<T> && other)
|
||||
{
|
||||
::new (data) ManuallyDrop<T>(other.take());
|
||||
}
|
||||
|
||||
~ManuallyDrop() {}
|
||||
|
||||
// FIXME(jade): do some "deducing this" nonsense to implement all the const
|
||||
// whatevers for this class. my clangd didn't like it when i tried, so that
|
||||
// is Later Work. this language is horrific.
|
||||
|
||||
/** Gets a reference to the inner T */
|
||||
T & get()
|
||||
{
|
||||
// SAFETY: this should genuinely never happen and it doesn't matter the
|
||||
// ordering of other stuff relative to it.
|
||||
assert(!destroyed.load(std::memory_order_relaxed));
|
||||
return reinterpret_cast<T &>(data);
|
||||
}
|
||||
|
||||
T & operator*()
|
||||
{
|
||||
return get();
|
||||
}
|
||||
|
||||
T * operator->()
|
||||
{
|
||||
return &get();
|
||||
}
|
||||
|
||||
/**
|
||||
* Takes the value out of this object and gives it to you.
|
||||
* Must not already be destroyed.
|
||||
*
|
||||
* Example:
|
||||
```c++
|
||||
ManuallyDrop<std::unique_ptr<int>> md{std::in_place_t{}, new int()};
|
||||
ManuallyDrop<std::unique_ptr<int>> md2{std::move(md).take()};
|
||||
```
|
||||
*/
|
||||
T && take() &&
|
||||
{
|
||||
// SAFETY: this is relatively lock-like in structure, reordering-wise
|
||||
bool wasDestroyed = destroyed.exchange(true, std::memory_order_acq_rel);
|
||||
assert(!wasDestroyed);
|
||||
return std::move(reinterpret_cast<T &>(data));
|
||||
}
|
||||
|
||||
/** Destroy the value. Safe to call multiple times. */
|
||||
void destroy()
|
||||
{
|
||||
// SAFETY: this is relatively lock-like in structure, reordering-wise
|
||||
bool wasDestroyed = destroyed.exchange(true, std::memory_order_acq_rel);
|
||||
if (!wasDestroyed) {
|
||||
get().~T();
|
||||
}
|
||||
}
|
||||
};
|
||||
}
|
||||
@@ -96,6 +96,7 @@ libutil_headers = files(
|
||||
'logging.hh',
|
||||
'logging-json.hh',
|
||||
'lru-cache.hh',
|
||||
'manually-drop.hh',
|
||||
'monitor-fd.hh',
|
||||
'mount.hh',
|
||||
'namespaces.hh',
|
||||
|
||||
Reference in New Issue
Block a user