From 4fc20b8d7d1dfc2f9973a7c12572a5b021c88f07 Mon Sep 17 00:00:00 2001 From: Maxime Coste Date: Sun, 4 Sep 2016 17:54:07 +0100 Subject: [PATCH] Rework client quitting and handling of remote errors Client quitting no longer immediately unwinds, client is just pushed for deletion until we get back to the main loop, similarly to what happens for buffer and window deletion. --- src/client.cc | 37 ++++---- src/client_manager.cc | 38 ++++---- src/client_manager.hh | 9 +- src/commands.cc | 14 +-- src/main.cc | 8 +- src/optional.hh | 2 + src/remote.cc | 88 +++++++++++-------- src/remote.hh | 7 +- test/highlight/regions/cmd | 2 +- .../638-highlight-codepoint-with-bracket/cmd | 2 +- 10 files changed, 107 insertions(+), 100 deletions(-) diff --git a/src/client.cc b/src/client.cc index 3d4e90dd..1d65227b 100644 --- a/src/client.cc +++ b/src/client.cc @@ -74,34 +74,27 @@ void Client::handle_available_input(EventMode mode) try { - try + while (Optional key = get_next_key(mode)) { - while (Optional key = get_next_key(mode)) + if (*key == ctrl('c')) + killpg(getpgrp(), SIGINT); + else if (*key == Key::FocusIn) + context().hooks().run_hook("FocusIn", context().name(), context()); + else if (*key == Key::FocusOut) + context().hooks().run_hook("FocusOut", context().name(), context()); + else if (key->modifiers == Key::Modifiers::Resize) { - if (*key == ctrl('c')) - killpg(getpgrp(), SIGINT); - else if (*key == Key::FocusIn) - context().hooks().run_hook("FocusIn", context().name(), context()); - else if (*key == Key::FocusOut) - context().hooks().run_hook("FocusOut", context().name(), context()); - else if (key->modifiers == Key::Modifiers::Resize) - { - m_window->set_dimensions(m_ui->dimensions()); - force_redraw(); - } - else - m_input_handler.handle_key(*key); + m_window->set_dimensions(m_ui->dimensions()); + force_redraw(); } - } - catch (Kakoune::runtime_error& error) - { - context().print_status({ error.what().str(), get_face("Error") }); - context().hooks().run_hook("RuntimeError", error.what(), context()); + else + m_input_handler.handle_key(*key); } } - catch (Kakoune::client_removed& removed) + catch (Kakoune::runtime_error& error) { - ClientManager::instance().remove_client(*this, removed.graceful); + context().print_status({ error.what().str(), get_face("Error") }); + context().hooks().run_hook("RuntimeError", error.what(), context()); } } diff --git a/src/client_manager.cc b/src/client_manager.cc index 96165538..d7523ff2 100644 --- a/src/client_manager.cc +++ b/src/client_manager.cc @@ -23,6 +23,7 @@ void ClientManager::clear() // So that clients destructor find the client manager empty // so that local UI does not fork. ClientList clients = std::move(m_clients); + m_client_trash.clear(); } String ClientManager::generate_name() const @@ -47,24 +48,17 @@ Client* ClientManager::create_client(std::unique_ptr&& ui, m_clients.emplace_back(client); try { - try - { - CommandManager::instance().execute(init_commands, client->context()); - } - catch (Kakoune::runtime_error& error) - { - client->context().print_status({ error.what().str(), get_face("Error") }); - client->context().hooks().run_hook("RuntimeError", error.what(), - client->context()); - } + CommandManager::instance().execute(init_commands, client->context()); } - catch (Kakoune::client_removed& removed) + catch (Kakoune::runtime_error& error) { - remove_client(*client, removed.graceful); - return nullptr; + client->context().print_status({ error.what().str(), get_face("Error") }); + client->context().hooks().run_hook("RuntimeError", error.what(), + client->context()); } - return client; + // Do not return the client if it already got moved to the trash + return contains(m_clients, client) ? client : nullptr; } void ClientManager::handle_pending_inputs() const @@ -75,10 +69,13 @@ void ClientManager::handle_pending_inputs() const void ClientManager::remove_client(Client& client, bool graceful) { - auto it = find_if(m_clients, - [&](const std::unique_ptr& ptr) - { return ptr.get() == &client; }); - kak_assert(it != m_clients.end()); + auto it = find(m_clients, &client); + if (it == m_clients.end()) + { + kak_assert(contains(m_client_trash, &client)); + return; + } + m_client_trash.push_back(std::move(*it)); m_clients.erase(it); if (not graceful and m_clients.empty()) @@ -155,6 +152,11 @@ void ClientManager::clear_window_trash() m_window_trash.clear(); } +void ClientManager::clear_client_trash() +{ + m_client_trash.clear(); +} + bool ClientManager::validate_client_name(StringView name) const { return const_cast(this)->get_client_ifp(name) == nullptr; diff --git a/src/client_manager.hh b/src/client_manager.hh index 7f2b603c..b7a085a2 100644 --- a/src/client_manager.hh +++ b/src/client_manager.hh @@ -7,13 +7,6 @@ namespace Kakoune { -struct client_removed -{ - client_removed(bool graceful) : graceful{graceful} {} - - const bool graceful; -}; - struct WindowAndSelections { std::unique_ptr window; @@ -58,10 +51,12 @@ public: ByteCount cursor_pos = -1) const; void clear_window_trash(); + void clear_client_trash(); private: String generate_name() const; ClientList m_clients; + ClientList m_client_trash; Vector m_free_windows; Vector> m_window_trash; }; diff --git a/src/commands.cc b/src/commands.cc index 56b8346c..b8cf4c1c 100644 --- a/src/commands.cc +++ b/src/commands.cc @@ -344,12 +344,12 @@ const CommandDesc force_kill_cmd = { }; template -void quit() +void quit(Context& context) { if (not force and ClientManager::instance().count() == 1) ensure_all_buffers_are_saved(); - // unwind back to this client event handler. - throw client_removed{ true }; + + ClientManager::instance().remove_client(context.client(), true); } const CommandDesc quit_cmd = { @@ -361,7 +361,7 @@ const CommandDesc quit_cmd = { CommandFlags::None, CommandHelper{}, CommandCompleter{}, - [](const ParametersParser&, Context&, const ShellContext&){ quit(); } + [](const ParametersParser&, Context& context, const ShellContext&){ quit(context); } }; const CommandDesc force_quit_cmd = { @@ -374,7 +374,7 @@ const CommandDesc force_quit_cmd = { CommandFlags::None, CommandHelper{}, CommandCompleter{}, - [](const ParametersParser&, Context&, const ShellContext&){ quit(); } + [](const ParametersParser&, Context& context, const ShellContext&){ quit(context); } }; template @@ -382,7 +382,7 @@ void write_quit(const ParametersParser& parser, Context& context, const ShellContext& shell_context) { write_buffer(parser, context, shell_context); - quit(); + quit(context); } const CommandDesc write_quit_cmd = { @@ -419,7 +419,7 @@ const CommandDesc writeall_quit_cmd = { [](const ParametersParser& parser, Context& context, const ShellContext&) { write_all_buffers(); - quit(); + quit(context); } }; diff --git a/src/main.cc b/src/main.cc index e8fd512a..dbaae037 100644 --- a/src/main.cc +++ b/src/main.cc @@ -451,7 +451,7 @@ int run_client(StringView session, StringView init_command, UIType ui_type) while (true) event_manager.handle_next_events(EventMode::Normal); } - catch (peer_disconnected&) + catch (remote_error&) { write_stderr("disconnected from server\n"); return -1; @@ -524,11 +524,6 @@ int run_server(StringView session, StringView init_command, startup_error = true; write_to_debug_buffer(format("error while parsing kakrc:\n {}", error.what())); } - catch (Kakoune::client_removed&) - { - startup_error = true; - write_to_debug_buffer("error while parsing kakrc: asked to quit"); - } { Context empty_context{Context::EmptyContextFlag{}}; @@ -589,6 +584,7 @@ int run_server(StringView session, StringView init_command, client_manager.redraw_clients(); event_manager.handle_next_events(EventMode::Normal); client_manager.handle_pending_inputs(); + client_manager.clear_client_trash(); client_manager.clear_window_trash(); buffer_manager.clear_buffer_trash(); string_registry.purge_unused(); diff --git a/src/optional.hh b/src/optional.hh index 5aecf3b3..54c72a89 100644 --- a/src/optional.hh +++ b/src/optional.hh @@ -80,6 +80,8 @@ public: template T value_or(U&& fallback) const { return m_valid ? m_value : T{fallback}; } + void reset() { destruct_ifn(); m_valid = false; } + private: void destruct_ifn() { if (m_valid) m_value.~T(); } diff --git a/src/remote.cc b/src/remote.cc index f22631ca..324ea8ce 100644 --- a/src/remote.cc +++ b/src/remote.cc @@ -54,7 +54,7 @@ public: *reinterpret_cast(m_stream.data()+1) = (uint32_t)m_stream.size(); int res = ::write(m_socket, m_stream.data(), m_stream.size()); if (res == 0) - throw peer_disconnected{}; + throw remote_error{"peer disconnected"}; } void write(const char* val, size_t size) @@ -171,8 +171,8 @@ public: void read(char* buffer, size_t size) { - if (m_stream.size() - m_read_pos < size) - throw peer_disconnected{}; + if (m_read_pos + size > m_stream.size()) + throw remote_error{"tried to read after message end"}; memcpy(buffer, m_stream.data() + m_read_pos, size); m_read_pos += size; } @@ -229,7 +229,7 @@ private: { int res = ::read(sock, m_stream.data() + m_write_pos, size); if (res == 0) - throw peer_disconnected{}; + throw remote_error{"peer disconnected"}; if (res < 0) throw socket_error{}; m_write_pos += res; @@ -325,23 +325,51 @@ public: void set_ui_options(const Options& options) override; + void set_client(Client* client) { m_client = client; } + Client* client() const { return m_client.get(); } + private: FDWatcher m_socket_watcher; MsgReader m_reader; CharCoord m_dimensions; InputCallback m_input_callback; + + SafePtr m_client; + Optional m_pending_key; }; RemoteUI::RemoteUI(int socket, CharCoord dimensions) : m_socket_watcher(socket, [this](FDWatcher& watcher, EventMode mode) { - const int sock = watcher.fd(); - while (fd_readable(sock) and not m_reader.ready()) - m_reader.read_available(sock); + const int sock = watcher.fd(); + bool disconnect = false; + try + { + while (fd_readable(sock) and not m_reader.ready()) + m_reader.read_available(sock); - if (m_reader.ready() and m_input_callback) - m_input_callback(mode); - }), + if (m_reader.ready() and not m_pending_key) + { + if (m_reader.type() == MessageType::Key) + { + m_pending_key = m_reader.read(); + m_reader.reset(); + } + else + disconnect = true; + } + } + catch (remote_error& err) + { + write_to_debug_buffer(format("Error while reading remote message: {}", err.what())); + disconnect = true; + } + + if (disconnect) + ClientManager::instance().remove_client(*m_client, false); + else if (m_pending_key and m_input_callback) + m_input_callback(mode); + }), m_dimensions(dimensions) { write_to_debug_buffer(format("remote client connected: {}", m_socket_watcher.fd())); @@ -427,32 +455,17 @@ void RemoteUI::set_ui_options(const Options& options) bool RemoteUI::is_key_available() { - return m_reader.ready(); + return (bool)m_pending_key; } Key RemoteUI::get_key() { - kak_assert(m_reader.ready()); - try - { - if (m_reader.type() != MessageType::Key) - throw client_removed{ false }; - - Key key = m_reader.read(); - m_reader.reset(); - if (key.modifiers == Key::Modifiers::Resize) - m_dimensions = key.coord(); - return key; - } - catch (peer_disconnected&) - { - throw client_removed{ false }; - } - catch (socket_error&) - { - write_to_debug_buffer("ungraceful deconnection detected"); - throw client_removed{ false }; - } + kak_assert(m_pending_key); + auto key = *m_pending_key; + m_pending_key.reset(); + if (key.modifiers == Key::Modifiers::Resize) + m_dimensions = key.coord(); + return key; } CharCoord RemoteUI::dimensions() @@ -632,10 +645,12 @@ private: auto init_command = m_reader.read(); auto dimensions = m_reader.read(); auto env_vars = m_reader.read_idmap(); - std::unique_ptr ui{new RemoteUI{sock, dimensions}}; - ClientManager::instance().create_client(std::move(ui), - std::move(env_vars), - init_command); + RemoteUI* ui = new RemoteUI{sock, dimensions}; + if (auto* client = ClientManager::instance().create_client( + std::unique_ptr{ui}, + std::move(env_vars), init_command)) + ui->set_client(client); + Server::instance().remove_accepter(this); return; } @@ -652,7 +667,6 @@ private: write_to_debug_buffer(format("error running command '{}': {}", command, e.what())); } - catch (client_removed&) {} close(sock); Server::instance().remove_accepter(this); return; diff --git a/src/remote.hh b/src/remote.hh index 27bc5f8a..5740def9 100644 --- a/src/remote.hh +++ b/src/remote.hh @@ -12,7 +12,12 @@ namespace Kakoune { -struct peer_disconnected {}; +struct remote_error : runtime_error +{ + remote_error(String error) + : runtime_error{std::move(error)} + {} +}; struct connection_failed : runtime_error { diff --git a/test/highlight/regions/cmd b/test/highlight/regions/cmd index db6cccd0..8b137891 100644 --- a/test/highlight/regions/cmd +++ b/test/highlight/regions/cmd @@ -1 +1 @@ -:q + diff --git a/test/regression/638-highlight-codepoint-with-bracket/cmd b/test/regression/638-highlight-codepoint-with-bracket/cmd index 977f45f1..e556b830 100644 --- a/test/regression/638-highlight-codepoint-with-bracket/cmd +++ b/test/regression/638-highlight-codepoint-with-bracket/cmd @@ -1 +1 @@ -w:q +w