From f0065cda13dff495cc2ac160f1cd1921425a30a2 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 21 Jan 2017 16:48:07 -0800 Subject: [PATCH] Clean up event_t handling Use shared_ptr instead of the silly killme list --- src/builtin.cpp | 4 +- src/complete.cpp | 1 - src/event.cpp | 139 ++++++++++++++++------------------------------- src/event.h | 7 +-- 4 files changed, 51 insertions(+), 100 deletions(-) diff --git a/src/builtin.cpp b/src/builtin.cpp index 59a97f60b..12252bde5 100644 --- a/src/builtin.cpp +++ b/src/builtin.cpp @@ -944,7 +944,7 @@ static wcstring functions_def(const wcstring &name) { function_get_definition(name, &def); event_t search(EVENT_ANY); search.function_name = name; - std::vector ev; + std::vector> ev; event_get(search, &ev); out.append(L"function "); @@ -967,7 +967,7 @@ static wcstring functions_def(const wcstring &name) { } for (size_t i = 0; i < ev.size(); i++) { - event_t *next = ev.at(i); + const event_t *next = ev.at(i).get(); switch (next->type) { case EVENT_SIGNAL: { append_format(out, L" --on-signal %ls", sig2wcs(next->param1.signal)); diff --git a/src/complete.cpp b/src/complete.cpp index 0494341a0..5bb47aff0 100644 --- a/src/complete.cpp +++ b/src/complete.cpp @@ -479,7 +479,6 @@ void complete_remove(const wcstring &cmd, bool cmd_is_path, const wcstring &opti bool delete_it = entry.remove_option(option, type); if (delete_it) { - // Delete this entry. completion_set.erase(iter); } } diff --git a/src/event.cpp b/src/event.cpp index ed72f283c..3bfda8cc8 100644 --- a/src/event.cpp +++ b/src/event.cpp @@ -39,12 +39,10 @@ static signal_list_t sig_list[2] = {{}, {}}; /// The index of sig_list that is the list of signals currently written to. static volatile int active_list = 0; -typedef std::vector event_list_t; +typedef std::vector> event_list_t; /// List of event handlers. static event_list_t s_event_handlers; -/// List of event handlers that should be removed. -static event_list_t killme; /// List of events that have been sent but have not yet been delivered because they are blocked. static event_list_t blocked; @@ -242,61 +240,51 @@ static wcstring event_desc_compact(const event_t &event) { } void event_add_handler(const event_t &event) { - event_t *e; - if (debug_level >= 3) { wcstring desc = event_desc_compact(event); debug(3, "register: %ls\n", desc.c_str()); } - e = new event_t(event); + shared_ptr e = std::make_shared(event); if (e->type == EVENT_SIGNAL) { signal_handle(e->param1.signal, 1); set_signal_observed(e->param1.signal, true); } - s_event_handlers.push_back(e); + s_event_handlers.push_back(std::move(e)); } void event_remove(const event_t &criterion) { - event_list_t new_list; - if (debug_level >= 3) { wcstring desc = event_desc_compact(criterion); debug(3, "unregister: %ls\n", desc.c_str()); } - // Because of concurrency issues (env_remove could remove an event that is currently being - // executed), env_remove does not actually free any events - instead it simply moves all events - // that should be removed from the event list to the killme list, and the ones that shouldn't be - // killed to new_list, and then drops the empty events-list. - if (s_event_handlers.empty()) return; - - for (size_t i = 0; i < s_event_handlers.size(); i++) { - event_t *n = s_event_handlers.at(i); - if (event_match(criterion, *n)) { - killme.push_back(n); - - // If this event was a signal handler and no other handler handles the specified signal - // type, do not handle that type of signal any more. - if (n->type == EVENT_SIGNAL) { - event_t e = event_t::signal_event(n->param1.signal); - if (event_get(e, 0) == 1) { - signal_handle(e.param1.signal, 0); - set_signal_observed(e.param1.signal, 0); - } - } - } else { - new_list.push_back(n); + event_list_t::iterator iter = s_event_handlers.begin(); + while (iter != s_event_handlers.end()) { + const event_t *n = iter->get(); + if (! event_match(criterion, *n)) { + ++iter; + continue; } + + // If this event was a signal handler and no other handler handles the specified signal + // type, do not handle that type of signal any more. + if (n->type == EVENT_SIGNAL) { + event_t e = event_t::signal_event(n->param1.signal); + if (event_get(e, NULL) == 1) { + signal_handle(e.param1.signal, 0); + set_signal_observed(e.param1.signal, 0); + } + } + iter = s_event_handlers.erase(iter); } - s_event_handlers.swap(new_list); } -int event_get(const event_t &criterion, std::vector *out) { +int event_get(const event_t &criterion, event_list_t *out) { + ASSERT_IS_MAIN_THREAD(); int found = 0; - for (size_t i = 0; i < s_event_handlers.size(); i++) { - event_t *n = s_event_handlers.at(i); + for (const shared_ptr &n : s_event_handlers) { if (event_match(criterion, *n)) { found++; if (out) out->push_back(n); @@ -314,17 +302,6 @@ bool event_is_signal_observed(int sig) { return result; } -/// Free all events in the kill list. -static void event_free_kills() { - for_each(killme.begin(), killme.end(), event_free); - killme.resize(0); -} - -/// Test if the specified event is waiting to be killed. -static int event_is_killed(const event_t &e) { - return std::find(killme.begin(), killme.end(), &e) != killme.end(); -} - /// Callback for firing (and then deleting) an event. static void fire_event_callback(void *arg) { ASSERT_IS_MAIN_THREAD(); @@ -338,20 +315,11 @@ static void fire_event_callback(void *arg) { /// event handler, we make sure to optimize the 'no matches' path. This means that nothing is /// allocated/initialized unless needed. static void event_fire_internal(const event_t &event) { - event_list_t fire; - - // First we free all events that have been removed, but only if this invocation of - // event_fire_internal is not a recursive call. - if (is_event <= 1) event_free_kills(); - - if (s_event_handlers.empty()) return; - - // Then we iterate over all events, adding events that should be fired to a second list. We need + // Iterate over all events, adding events that should be fired to a second list. We need // to do this in a separate step since an event handler might call event_remove or // event_add_handler, which will change the contents of the \c events list. - for (size_t i = 0; i < s_event_handlers.size(); i++) { - event_t *criterion = s_event_handlers.at(i); - + event_list_t fire; + for (shared_ptr &criterion : s_event_handlers) { // Check if this event is a match. if (event_match(*criterion, event)) { fire.push_back(criterion); @@ -370,12 +338,13 @@ static void event_fire_internal(const event_t &event) { } // Iterate over our list of matching events. - for (size_t i = 0; i < fire.size(); i++) { - event_t *criterion = fire.at(i); - int prev_status; - - // Check if this event has been removed, if so, dont fire it. - if (event_is_killed(*criterion)) continue; + for (shared_ptr &criterion : fire) { + // Only fire if this event is still present + if (std::find(s_event_handlers.begin(), + s_event_handlers.end(), + criterion) == s_event_handlers.end()) { + continue; + } // Fire event. wcstring buffer = criterion->function_name; @@ -391,7 +360,7 @@ static void event_fire_internal(const event_t &event) { // Event handlers are not part of the main flow of code, so they are marked as // non-interactive. proc_push_interactive(0); - prev_status = proc_get_last_status(); + int prev_status = proc_get_last_status(); parser_t &parser = parser_t::principal_parser(); event_block_t *b = parser.push_block(event); @@ -400,9 +369,6 @@ static void event_fire_internal(const event_t &event) { proc_pop_interactive(); proc_set_last_status(prev_status); } - - // Free killed events. - if (is_event <= 1) event_free_kills(); } /// Handle all pending signal events. @@ -412,18 +378,15 @@ static void event_fire_delayed() { // When the event handler has called a piece of code that triggers another event, we do not want // to fire delayed events because of concurrency problems. if (!blocked.empty() && is_event == 1) { - event_list_t new_blocked; - - for (size_t i = 0; i < blocked.size(); i++) { - event_t *e = blocked.at(i); + event_list_t local_blocked; + local_blocked.swap(blocked); + for (const shared_ptr &e : local_blocked) { if (event_is_blocked(*e)) { - new_blocked.push_back(new event_t(*e)); + blocked.push_back(e); } else { event_fire_internal(*e); - event_free(e); } } - blocked.swap(new_blocked); } int al = active_list; @@ -439,21 +402,20 @@ static void event_fire_delayed() { // Set up. lst = &sig_list[1 - al]; - event_t e = event_t::signal_event(0); - e.arguments.resize(1); - if (lst->overflow) { debug(0, _(L"Signal list overflow. Signals have been ignored.")); } // Send all signals in our private list. for (int i = 0; i < lst->count; i++) { - e.param1.signal = lst->signal[i]; - e.arguments.at(0) = sig2wcs(e.param1.signal); - if (event_is_blocked(e)) { - blocked.push_back(new event_t(e)); + shared_ptr e = std::make_shared(EVENT_SIGNAL); + int signal = lst->signal[i]; + e->param1.signal = signal; + e->arguments.push_back(sig2wcs(signal)); + if (event_is_blocked(*e)) { + blocked.push_back(e); } else { - event_fire_internal(e); + event_fire_internal(*e); } } } @@ -479,7 +441,7 @@ void event_fire(const event_t *event) { if (event) { if (event_is_blocked(*event)) { - blocked.push_back(new event_t(*event)); + blocked.push_back(std::make_shared(*event)); } else { event_fire_internal(*event); } @@ -491,16 +453,7 @@ void event_fire(const event_t *event) { void event_init() {} void event_destroy() { - for_each(s_event_handlers.begin(), s_event_handlers.end(), event_free); s_event_handlers.clear(); - - for_each(killme.begin(), killme.end(), event_free); - killme.clear(); -} - -void event_free(event_t *e) { - CHECK(e, ); - delete e; } void event_fire_generic(const wchar_t *name, wcstring_list_t *args) { diff --git a/src/event.h b/src/event.h index 077731c7a..809489f44 100644 --- a/src/event.h +++ b/src/event.h @@ -7,6 +7,7 @@ #define FISH_EVENT_H #include +#include #include #include "common.h" @@ -100,7 +101,7 @@ void event_remove(const event_t &event); /// result count will still be valid /// /// \return the number of found matches -int event_get(const event_t &criterion, std::vector *out); +int event_get(const event_t &criterion, std::vector> *out); /// Returns whether an event listener is registered for the given signal. This is safe to call from /// a signal handler. @@ -120,6 +121,7 @@ bool event_is_signal_observed(int signal); void event_fire(const event_t *event); /// Like event_fire, but takes a signal directly. +/// May be called from signal handlers void event_fire_signal(int signal); /// Initialize the event-handling library. @@ -128,9 +130,6 @@ void event_init(); /// Destroy the event-handling library. void event_destroy(); -/// Free all memory used by the specified event. -void event_free(event_t *e); - /// Returns a string describing the specified event. wcstring event_get_desc(const event_t &e);