Track histories with shared_ptr

Prior to this change, histories were immortal and allocated with either
unique_ptr or just leaked via new. But this can result in races in the
path detection test, as the destructor races with the pointer-captured
history. Switch to using shared_ptr.
This commit is contained in:
ridiculousfish 2021-01-09 16:22:32 -08:00
parent e062a07a97
commit e8c9da100c
10 changed files with 100 additions and 85 deletions

View File

@ -216,8 +216,8 @@ maybe_t<int> builtin_history(parser_t &parser, io_streams_t &streams, wchar_t **
// Use the default history if we have none (which happens if invoked non-interactively, e.g.
// from webconfig.py.
history_t *history = reader_get_history();
if (!history) history = &history_t::history_with_name(history_session_id(parser.vars()));
std::shared_ptr<history_t> history = reader_get_history();
if (!history) history = history_t::with_name(history_session_id(parser.vars()));
// If a history command hasn't already been specified via a flag check the first word.
// Note that this can be simplified after we eliminate allowing subcommands as flags.

View File

@ -483,8 +483,8 @@ static int builtin_set_list(const wchar_t *cmd, set_cmd_opts_t &opts, int argc,
if (!names_only) {
wcstring val;
if (opts.shorten_ok && key == L"history") {
history_t *history =
&history_t::history_with_name(history_session_id(parser.vars()));
std::shared_ptr<history_t> history =
history_t::with_name(history_session_id(parser.vars()));
for (size_t i = 1; i < history->size() && val.size() < 64; i++) {
if (i > 1) val += L' ';
val += expand_escape_string(history->item_at_index(i).str());

View File

@ -1252,8 +1252,8 @@ bool completer_t::complete_variable(const wcstring &str, size_t start_offset) {
// $history can be huge, don't put all of it in the completion description; see
// #6288.
if (env_name == L"history") {
history_t *history =
&history_t::history_with_name(history_session_id(ctx.vars));
std::shared_ptr<history_t> history =
history_t::with_name(history_session_id(ctx.vars));
for (size_t i = 1; i < history->size() && desc.size() < 64; i++) {
if (i > 1) desc += L' ';
desc += expand_escape_string(history->item_at_index(i).str());

View File

@ -687,9 +687,9 @@ maybe_t<env_var_t> env_scoped_impl_t::try_get_computed(const wcstring &key) cons
return none();
}
history_t *history = reader_get_history();
std::shared_ptr<history_t> history = reader_get_history();
if (!history) {
history = &history_t::history_with_name(history_session_id(*this));
history = history_t::with_name(history_session_id(*this));
}
wcstring_list_t result;
if (history) history->get_history(result);

View File

@ -355,13 +355,13 @@ static expand_result_t expand_variables(wcstring instr, completion_receiver_t *o
// Do a dirty hack to make sliced history fast (#4650). We expand from either a variable, or a
// history_t. Note that "history" is read only in env.cpp so it's safe to special-case it in
// this way (it cannot be shadowed, etc).
history_t *history = nullptr;
std::shared_ptr<history_t> history{};
maybe_t<env_var_t> var{};
if (var_name == L"history") {
// Note reader_get_history may return null, if we are running non-interactively (e.g. from
// web_config).
if (is_main_thread()) {
history = &history_t::history_with_name(history_session_id(env_stack_t::principal()));
history = history_t::with_name(history_session_id(env_stack_t::principal()));
}
} else if (var_name != wcstring{VARIABLE_EXPAND_EMPTY}) {
var = vars.get(var_name);

View File

@ -3508,7 +3508,7 @@ static bool history_contains(history_t *history, const wcstring &txt) {
return result;
}
static bool history_contains(const std::unique_ptr<history_t> &history, const wcstring &txt) {
static bool history_contains(const std::shared_ptr<history_t> &history, const wcstring &txt) {
return history_contains(history.get(), txt);
}
@ -4007,10 +4007,10 @@ void history_tests_t::test_history() {
const history_search_flags_t nocase = history_search_ignore_case;
// Populate a history.
history_t &history = history_t::history_with_name(L"test_history");
history.clear();
std::shared_ptr<history_t> history = history_t::with_name(L"test_history");
history->clear();
for (const wcstring &s : items) {
history.add(s);
history->add(s);
}
// Helper to set expected items to those matching a predicate, in reverse order.
@ -4062,13 +4062,13 @@ void history_tests_t::test_history() {
// Test item removal case-sensitive.
searcher = history_search_t(history, L"Alpha");
test_history_matches(searcher, {L"Alpha"}, __LINE__);
history.remove(L"Alpha");
history->remove(L"Alpha");
searcher = history_search_t(history, L"Alpha");
test_history_matches(searcher, {}, __LINE__);
// Test history escaping and unescaping, yaml, etc.
history_item_list_t before, after;
history.clear();
history->clear();
size_t i, max = 100;
for (i = 1; i <= max; i++) {
// Generate a value.
@ -4088,17 +4088,17 @@ void history_tests_t::test_history() {
history_item_t item(value, time(NULL));
item.required_paths = paths;
before.push_back(item);
history.add(item);
history->add(item);
}
history.save();
history->save();
// Empty items should just be dropped (#6032).
history.add(L"");
do_test(!history.item_at_index(1).contents.empty());
history->add(L"");
do_test(!history->item_at_index(1).contents.empty());
// Read items back in reverse order and ensure they're the same.
for (i = 100; i >= 1; i--) {
history_item_t item = history.item_at_index(i);
history_item_t item = history->item_at_index(i);
do_test(!item.empty());
after.push_back(item);
}
@ -4111,7 +4111,7 @@ void history_tests_t::test_history() {
}
// Clean up after our tests.
history.clear();
history->clear();
}
// Wait until the next second.
@ -4245,8 +4245,9 @@ void history_tests_t::test_history_merge() {
say(L"Testing history merge");
const size_t count = 3;
const wcstring name = L"merge_test";
std::unique_ptr<history_t> hists[count] = {
make_unique<history_t>(name), make_unique<history_t>(name), make_unique<history_t>(name)};
std::shared_ptr<history_t> hists[count] = {std::make_shared<history_t>(name),
std::make_shared<history_t>(name),
std::make_shared<history_t>(name)};
const wcstring texts[count] = {L"History 1", L"History 2", L"History 3"};
const wcstring alt_texts[count] = {L"History Alt 1", L"History Alt 2", L"History Alt 3"};
@ -4280,7 +4281,7 @@ void history_tests_t::test_history_merge() {
// Make a new history. It should contain everything. The time_barrier() is so that the timestamp
// is newer, since we only pick up items whose timestamp is before the birth stamp.
time_barrier();
std::unique_ptr<history_t> everything = make_unique<history_t>(name);
std::shared_ptr<history_t> everything = std::make_shared<history_t>(name);
for (const auto &text : texts) {
do_test(history_contains(everything, text));
}
@ -4357,22 +4358,23 @@ void history_tests_t::test_history_path_detection() {
vars->vars[L"PWD"] = tmpdir;
vars->vars[L"HOME"] = tmpdir;
history_t &history = history_t::history_with_name(L"path_detection");
history.add_pending_with_file_detection(L"cmd0 not/a/valid/path", vars);
history.add_pending_with_file_detection(L"cmd1 " + filename, vars);
history.add_pending_with_file_detection(L"cmd2 " + tmpdir + L"/" + filename, vars);
history.add_pending_with_file_detection(L"cmd3 $HOME/" + filename, vars);
history.add_pending_with_file_detection(L"cmd4 $HOME/notafile", vars);
history.add_pending_with_file_detection(L"cmd5 ~/" + filename, vars);
history.add_pending_with_file_detection(L"cmd6 ~/notafile", vars);
history.add_pending_with_file_detection(L"cmd7 ~/*f*", vars);
history.add_pending_with_file_detection(L"cmd8 ~/*zzz*", vars);
history.resolve_pending();
std::shared_ptr<history_t> history = history_t::with_name(L"path_detection");
history_t::add_pending_with_file_detection(history, L"cmd0 not/a/valid/path", vars);
history_t::add_pending_with_file_detection(history, L"cmd1 " + filename, vars);
history_t::add_pending_with_file_detection(history, L"cmd2 " + tmpdir + L"/" + filename, vars);
history_t::add_pending_with_file_detection(history, L"cmd3 $HOME/" + filename, vars);
history_t::add_pending_with_file_detection(history, L"cmd4 $HOME/notafile", vars);
history_t::add_pending_with_file_detection(history, L"cmd5 ~/" + filename, vars);
history_t::add_pending_with_file_detection(history, L"cmd6 ~/notafile", vars);
history_t::add_pending_with_file_detection(history, L"cmd7 ~/*f*", vars);
history_t::add_pending_with_file_detection(history, L"cmd8 ~/*zzz*", vars);
history->resolve_pending();
constexpr size_t hist_size = 9;
if (history.size() != hist_size) {
err(L"history has wrong size: %lu but expected %lu", (unsigned long)history.size(), (unsigned long)hist_size);
history.clear();
if (history->size() != hist_size) {
err(L"history has wrong size: %lu but expected %lu", (unsigned long)history->size(),
(unsigned long)hist_size);
history->clear();
return;
}
@ -4395,7 +4397,7 @@ void history_tests_t::test_history_path_detection() {
int failures = 0;
bool last = (lap + 1 == maxlap);
for (size_t i = 1; i <= hist_size; i++) {
if (history.item_at_index(i).required_paths != expected[hist_size - i]) {
if (history->item_at_index(i).required_paths != expected[hist_size - i]) {
failures += 1;
if (last) {
err(L"Wrong detected paths for item %lu", (unsigned long)i);
@ -4410,7 +4412,7 @@ void history_tests_t::test_history_path_detection() {
usleep(1E6 / 500); // 1 msec
}
//fprintf(stderr, "History saving took %lu laps\n", (unsigned long)lap);
history.clear();
history->clear();
}
static bool install_sample_history(const wchar_t *name) {
@ -4429,7 +4431,7 @@ static bool install_sample_history(const wchar_t *name) {
}
/// Indicates whether the history is equal to the given null-terminated array of strings.
static bool history_equals(history_t &hist, const wchar_t *const *strings) {
static bool history_equals(const shared_ptr<history_t> &hist, const wchar_t *const *strings) {
// Count our expected items.
size_t expected_count = 0;
while (strings[expected_count]) {
@ -4441,7 +4443,7 @@ static bool history_equals(history_t &hist, const wchar_t *const *strings) {
size_t array_idx = 0;
for (;;) {
const wchar_t *expected = strings[array_idx];
history_item_t item = hist.item_at_index(history_idx);
history_item_t item = hist->item_at_index(history_idx);
if (expected == NULL) {
if (!item.empty()) {
err(L"Expected empty item at history index %lu, instead found: %ls", history_idx, item.str().c_str());
@ -4473,11 +4475,11 @@ void history_tests_t::test_history_formats() {
const wchar_t *const expected[] = {
L"#def", L"echo #abc", L"function yay\necho hi\nend", L"cd foobar", L"ls /", NULL};
history_t &test_history = history_t::history_with_name(name);
auto test_history = history_t::with_name(name);
if (!history_equals(test_history, expected)) {
err(L"test_history_formats failed for %ls\n", name);
}
test_history.clear();
test_history->clear();
}
name = L"history_sample_fish_2_0";
@ -4488,11 +4490,11 @@ void history_tests_t::test_history_formats() {
const wchar_t *const expected[] = {L"echo this has\\\nbackslashes",
L"function foo\necho bar\nend", L"echo alpha", NULL};
history_t &test_history = history_t::history_with_name(name);
auto test_history = history_t::with_name(name);
if (!history_equals(test_history, expected)) {
err(L"test_history_formats failed for %ls\n", name);
}
test_history.clear();
test_history->clear();
}
say(L"Testing bash import");
@ -4506,12 +4508,12 @@ void history_tests_t::test_history_formats() {
L"/** # see issue 7407", L"sleep 123", L"a && echo valid construct",
L"final line", L"echo supsup", L"export XVAR='exported'",
L"history --help", L"echo foo", NULL};
history_t &test_history = history_t::history_with_name(L"bash_import");
test_history.populate_from_bash(f);
auto test_history = history_t::with_name(L"bash_import");
test_history->populate_from_bash(f);
if (!history_equals(test_history, expected)) {
err(L"test_history_formats failed for bash import\n");
}
test_history.clear();
test_history->clear();
fclose(f);
}
@ -4521,13 +4523,13 @@ void history_tests_t::test_history_formats() {
err(L"Couldn't open file tests/%ls", name);
} else {
// We simply invoke get_string_representation. If we don't die, the test is a success.
history_t &test_history = history_t::history_with_name(name);
auto test_history = history_t::with_name(name);
const wchar_t *expected[] = {L"no_newline_at_end_of_file", L"corrupt_prefix",
L"this_command_is_ok", NULL};
if (!history_equals(test_history, expected)) {
err(L"test_history_formats failed for %ls\n", name);
}
test_history.clear();
test_history->clear();
}
}

View File

@ -1324,9 +1324,13 @@ void history_t::remove(const wcstring &str) { impl()->remove(str); }
void history_t::remove_ephemeral_items() { impl()->remove_ephemeral_items(); }
void history_t::add_pending_with_file_detection(const wcstring &str,
// static
void history_t::add_pending_with_file_detection(const std::shared_ptr<history_t> &self,
const wcstring &str,
const std::shared_ptr<environment_t> &vars,
history_persistence_mode_t persist_mode) {
assert(self && "Null history");
// We use empty items as sentinels to indicate the end of history.
// Do not allow them to be added (#6032).
if (str.empty()) {
@ -1367,7 +1371,7 @@ void history_t::add_pending_with_file_detection(const wcstring &str,
// If we got a path, we'll perform file detection for autosuggestion hinting.
bool wants_file_detection = !potential_paths.empty() && !needs_sync_write;
auto imp = this->impl();
auto imp = self->impl();
// Make our history item.
time_t when = imp->timestamp_now();
@ -1384,7 +1388,7 @@ void history_t::add_pending_with_file_detection(const wcstring &str,
iothread_perform([=]() {
// Don't hold the lock while we perform this file detection.
auto validated_paths = expand_and_detect_paths(potential_paths, *vars);
auto imp = this->impl();
auto imp = self->impl();
imp->set_valid_file_paths(std::move(validated_paths), identifier);
imp->enable_automatic_saving();
});
@ -1404,7 +1408,7 @@ void history_t::save() { impl()->save(); }
/// Perform a search of \p hist for \p search_string. Invoke a function \p func for each match. If
/// \p func returns true, continue the search; else stop it.
static void do_1_history_search(history_t &hist, history_search_type_t search_type,
static void do_1_history_search(history_t *hist, history_search_type_t search_type,
const wcstring &search_string, bool case_sensitive,
const std::function<bool(const history_item_t &item)> &func,
const cancel_checker_t &cancel_check) {
@ -1443,7 +1447,7 @@ bool history_t::search(history_search_type_t search_type, const wcstring_list_t
if (search_args.empty()) {
// The user had no search terms; just append everything.
do_1_history_search(*this, history_search_type_t::match_everything, {}, false, func,
do_1_history_search(this, history_search_type_t::match_everything, {}, false, func,
cancel_check);
} else {
for (const wcstring &search_string : search_args) {
@ -1451,7 +1455,7 @@ bool history_t::search(history_search_type_t search_type, const wcstring_list_t
streams.err.append_format(L"Searching for the empty string isn't allowed");
return false;
}
do_1_history_search(*this, search_type, search_string, case_sensitive, func,
do_1_history_search(this, search_type, search_string, case_sensitive, func,
cancel_check);
}
}
@ -1482,7 +1486,7 @@ history_item_t history_t::item_at_index(size_t idx) { return impl()->item_at_ind
size_t history_t::size() { return impl()->size(); }
/// The set of all histories.
static owning_lock<std::map<wcstring, std::unique_ptr<history_t>>> s_histories;
static owning_lock<std::map<wcstring, std::shared_ptr<history_t>>> s_histories;
void history_save_all() {
auto histories = s_histories.acquire();
@ -1491,16 +1495,13 @@ void history_save_all() {
}
}
history_t &history_t::history_with_name(const wcstring &name) {
// Return a history for the given name, creating it if necessary
// Note that histories are currently never deleted, so we can return a reference to them without
// using something like shared_ptr
std::shared_ptr<history_t> history_t::with_name(const wcstring &name) {
auto hs = s_histories.acquire();
std::unique_ptr<history_t> &hist = (*hs)[name];
std::shared_ptr<history_t> &hist = (*hs)[name];
if (!hist) {
hist = make_unique<history_t>(name);
hist = std::make_shared<history_t>(name);
}
return *hist;
return hist;
}
void start_private_mode(env_stack_t &vars) {

View File

@ -163,7 +163,7 @@ class history_t {
static bool never_mmap;
// Returns history with the given name, creating it if necessary.
static history_t &history_with_name(const wcstring &name);
static std::shared_ptr<history_t> with_name(const wcstring &name);
/// Returns whether this is using the default name.
bool is_default() const;
@ -180,9 +180,10 @@ class history_t {
// Add a new pending history item to the end, and then begin file detection on the items to
// determine which arguments are paths. Arguments may be expanded (e.g. with PWD and variables)
// using the given \p vars. The item has the given \p persist_mode
void add_pending_with_file_detection(
const wcstring &str, const std::shared_ptr<environment_t> &vars,
// using the given \p vars. The item has the given \p persist_mode.
static void add_pending_with_file_detection(
const std::shared_ptr<history_t> &self, const wcstring &str,
const std::shared_ptr<environment_t> &vars,
history_persistence_mode_t persist_mode = history_persistence_mode_t::disk);
// Resolves any pending history items, so that they may be returned in history searches.
@ -241,6 +242,7 @@ using history_search_flags_t = uint32_t;
class history_search_t {
private:
// The history in which we are searching.
// TODO: this should be a shared_ptr.
history_t *history_;
// The original search term.
@ -283,16 +285,23 @@ class history_search_t {
// Returns the current search result item contents. asserts if there is no current item.
const wcstring &current_string() const;
// Constructor.
history_search_t(history_t &hist, const wcstring &str,
// Construct from a history pointer; the caller is responsible for ensuring the history stays
// alive.
history_search_t(history_t *hist, const wcstring &str,
enum history_search_type_t type = history_search_type_t::contains,
history_search_flags_t flags = 0)
: history_(&hist), orig_term_(str), canon_term_(str), search_type_(type), flags_(flags) {
: history_(hist), orig_term_(str), canon_term_(str), search_type_(type), flags_(flags) {
if (ignores_case()) {
std::transform(canon_term_.begin(), canon_term_.end(), canon_term_.begin(), towlower);
}
}
// Construct from a shared_ptr. TODO: this should be the only constructor.
history_search_t(const std::shared_ptr<history_t> &hist, const wcstring &str,
enum history_search_type_t type = history_search_type_t::contains,
history_search_flags_t flags = 0)
: history_search_t(hist.get(), str, type, flags) {}
// Default constructor.
history_search_t() = default;
};

View File

@ -446,7 +446,7 @@ class reader_history_search_t {
bool add_skip(const wcstring &str) { return skips_.insert(str).second; }
/// Reset, beginning a new line or token mode search.
void reset_to_mode(const wcstring &text, history_t *hist, mode_t mode) {
void reset_to_mode(const wcstring &text, const std::shared_ptr<history_t> &hist, mode_t mode) {
assert(mode != inactive && "mode cannot be inactive in this setter");
skips_ = {text};
matches_ = {text};
@ -458,7 +458,7 @@ class reader_history_search_t {
if (low == text) flags |= history_search_ignore_case;
// We can skip dedup in history_search_t because we do it ourselves in skips_.
search_ = history_search_t(
*hist, text,
hist, text,
by_prefix() ? history_search_type_t::prefix : history_search_type_t::contains, flags);
}
@ -586,7 +586,7 @@ class reader_data_t : public std::enable_shared_from_this<reader_data_t> {
/// The source of input events.
inputter_t inputter;
/// The history.
history_t *history{nullptr};
std::shared_ptr<history_t> history{};
/// The history search.
reader_history_search_t history_search{};
@ -680,11 +680,12 @@ class reader_data_t : public std::enable_shared_from_this<reader_data_t> {
/// Access the parser.
parser_t &parser() { return *parser_ref; }
reader_data_t(std::shared_ptr<parser_t> parser, history_t *hist, reader_config_t &&conf)
reader_data_t(std::shared_ptr<parser_t> parser, std::shared_ptr<history_t> hist,
reader_config_t &&conf)
: conf(std::move(conf)),
parser_ref(std::move(parser)),
inputter(*parser_ref, conf.in),
history(hist) {}
history(std::move(hist)) {}
void update_buff_pos(editable_line_t *el, maybe_t<size_t> new_pos = none_t());
@ -1630,7 +1631,8 @@ void reader_data_t::completion_insert(const wcstring &val, size_t token_end,
// Returns a function that can be invoked (potentially
// on a background thread) to determine the autosuggestion
static std::function<autosuggestion_t(void)> get_autosuggestion_performer(
parser_t &parser, const wcstring &search_string, size_t cursor_pos, history_t *history) {
parser_t &parser, const wcstring &search_string, size_t cursor_pos,
const std::shared_ptr<history_t> &history) {
const uint32_t generation_count = read_generation_count();
auto vars = parser.vars().snapshot();
const wcstring working_directory = vars->get_pwd_slash();
@ -1651,7 +1653,7 @@ static std::function<autosuggestion_t(void)> get_autosuggestion_performer(
}
// Search history for a matching item.
history_search_t searcher(*history, search_string, history_search_type_t::prefix,
history_search_t searcher(history.get(), search_string, history_search_type_t::prefix,
history_search_flags_t{});
while (!ctx.check_cancel() && searcher.go_backwards()) {
const history_item_t &item = searcher.current_item();
@ -2512,7 +2514,7 @@ void reader_change_history(const wcstring &name) {
reader_data_t *data = current_data_or_null();
if (data && data->history) {
data->history->save();
data->history = &history_t::history_with_name(name);
data->history = history_t::with_name(name);
}
}
@ -2521,7 +2523,7 @@ void reader_change_history(const wcstring &name) {
static std::shared_ptr<reader_data_t> reader_push_ret(parser_t &parser,
const wcstring &history_name,
reader_config_t &&conf) {
history_t *hist = &history_t::history_with_name(history_name);
std::shared_ptr<history_t> hist = history_t::with_name(history_name);
auto data = std::make_shared<reader_data_t>(parser.shared(), hist, std::move(conf));
reader_data_stack.push_back(data);
data->command_line_changed(&data->command_line);
@ -3241,7 +3243,8 @@ void reader_data_t::handle_readline_command(readline_cmd_t c, readline_loop_stat
} else {
mode = history_persistence_mode_t::disk;
}
history->add_pending_with_file_detection(text, vars.snapshot(), mode);
history_t::add_pending_with_file_detection(history, text, vars.snapshot(),
mode);
}
rls.finished = true;
@ -4061,7 +4064,7 @@ const wchar_t *reader_get_buffer() {
return data ? data->command_line.text().c_str() : nullptr;
}
history_t *reader_get_history() {
std::shared_ptr<history_t> reader_get_history() {
ASSERT_IS_MAIN_THREAD();
reader_data_t *data = current_data_or_null();
return data ? data->history : nullptr;

View File

@ -169,7 +169,7 @@ void reader_queue_ch(const char_event_t &ch);
const wchar_t *reader_get_buffer();
/// Returns the current reader's history.
history_t *reader_get_history();
std::shared_ptr<history_t> reader_get_history();
/// Set the string of characters in the command buffer, as well as the cursor position.
///