From 1cd0544a3f76ec8b26b136e9f824fbfa1ca1266d Mon Sep 17 00:00:00 2001 From: Andy Gayton Date: Tue, 6 Aug 2024 06:35:40 -0400 Subject: [PATCH] fix: relay Signals reset to plugins (#13510) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR will close #13501 # Description This PR expands on [the relay of signals to running plugin processes](https://github.com/nushell/nushell/pull/13181). The Ctrlc relay has been generalized to SignalAction::Interrupt and when reset_signal is called on the main EngineState, a SignalAction::Reset is now relayed to running plugins. # User-Facing Changes The signal handler closure now takes a `signals::SignalAction`, while previously it took no arguments. The handler will now be called on both interrupt and reset. The method to register a handler on the plugin side is now called `register_signal_handler` instead of `register_ctrlc_handler` [example](https://github.com/nushell/nushell/pull/13510/files#diff-3e04dff88fd0780a49778a3d1eede092ec729a1264b4ef07ca0d2baa859dad05L38). This will only affect plugin authors who have started making use of https://github.com/nushell/nushell/pull/13181, which isn't currently part of an official release. The change will also require all of user's plugins to be recompiled in order that they don't error when a signal is received on the PluginInterface. # Testing ``` : example ctrlc interrupt status: false waiting for interrupt signal... ^Cinterrupt status: true peace. Error: × Operation interrupted ╭─[display_output hook:1:1] 1 │ if (term size).columns >= 100 { table -e } else { table } · ─┬ · ╰── This operation was interrupted ╰──── : example ctrlc interrupt status: false <-- NOTE status is false waiting for interrupt signal... ^Cinterrupt status: true peace. Error: × Operation interrupted ╭─[display_output hook:1:1] 1 │ if (term size).columns >= 100 { table -e } else { table } · ─┬ · ╰── This operation was interrupted ╰──── ``` --- crates/nu-plugin-engine/src/interface/mod.rs | 8 ++--- crates/nu-plugin-engine/src/persistent.rs | 24 ++++++------- crates/nu-plugin-protocol/src/lib.rs | 7 ++-- crates/nu-plugin/src/plugin/interface/mod.rs | 35 ++++++++++--------- crates/nu-protocol/src/engine/engine_state.rs | 21 ++++++----- crates/nu-protocol/src/engine/mod.rs | 2 -- .../{engine/ctrlc.rs => pipeline/handlers.rs} | 30 ++++++++-------- crates/nu-protocol/src/pipeline/mod.rs | 2 ++ crates/nu-protocol/src/pipeline/signals.rs | 12 ++++++- crates/nu-protocol/src/plugin/registered.rs | 7 ++-- .../nu_plugin_example/src/commands/ctrlc.rs | 6 ++-- src/signals.rs | 11 +++--- 12 files changed, 86 insertions(+), 79 deletions(-) rename crates/nu-protocol/src/{engine/ctrlc.rs => pipeline/handlers.rs} (83%) diff --git a/crates/nu-plugin-engine/src/interface/mod.rs b/crates/nu-plugin-engine/src/interface/mod.rs index 78ed966dd6c8..b27ea0be3d9d 100644 --- a/crates/nu-plugin-engine/src/interface/mod.rs +++ b/crates/nu-plugin-engine/src/interface/mod.rs @@ -12,7 +12,7 @@ use nu_plugin_protocol::{ }; use nu_protocol::{ ast::Operator, engine::Sequence, CustomValue, IntoSpanned, PipelineData, PluginMetadata, - PluginSignature, ShellError, Signals, Span, Spanned, Value, + PluginSignature, ShellError, SignalAction, Signals, Span, Spanned, Value, }; use nu_utils::SharedCow; use std::{ @@ -664,9 +664,9 @@ impl PluginInterface { self.flush() } - /// Send the plugin a ctrl-c signal. - pub fn ctrlc(&self) -> Result<(), ShellError> { - self.write(PluginInput::Ctrlc)?; + /// Send the plugin a signal. + pub fn signal(&self, action: SignalAction) -> Result<(), ShellError> { + self.write(PluginInput::Signal(action))?; self.flush() } diff --git a/crates/nu-plugin-engine/src/persistent.rs b/crates/nu-plugin-engine/src/persistent.rs index 69f5c7f7a8c3..f9dcc5143b82 100644 --- a/crates/nu-plugin-engine/src/persistent.rs +++ b/crates/nu-plugin-engine/src/persistent.rs @@ -6,8 +6,9 @@ use crate::{ use super::{PluginInterface, PluginSource}; use nu_plugin_core::CommunicationMode; use nu_protocol::{ - engine::{ctrlc, EngineState, Stack}, - PluginGcConfig, PluginIdentity, PluginMetadata, RegisteredPlugin, ShellError, + engine::{EngineState, Stack}, + HandlerGuard, Handlers, PluginGcConfig, PluginIdentity, PluginMetadata, RegisteredPlugin, + ShellError, }; use std::{ collections::HashMap, @@ -37,8 +38,8 @@ struct MutableState { preferred_mode: Option, /// Garbage collector config gc_config: PluginGcConfig, - /// RAII guard for this plugin's ctrl-c handler - ctrlc_guard: Option, + /// RAII guard for this plugin's signal handler + signal_guard: Option, } #[derive(Debug, Clone, Copy)] @@ -66,7 +67,7 @@ impl PersistentPlugin { metadata: None, preferred_mode: None, gc_config, - ctrlc_guard: None, + signal_guard: None, }), } } @@ -303,21 +304,18 @@ impl RegisteredPlugin for PersistentPlugin { self } - fn configure_ctrlc_handler( - self: Arc, - handlers: &ctrlc::Handlers, - ) -> Result<(), ShellError> { + fn configure_signal_handler(self: Arc, handlers: &Handlers) -> Result<(), ShellError> { let guard = { // We take a weakref to the plugin so that we don't create a cycle to the // RAII guard that will be stored on the plugin. let plugin = Arc::downgrade(&self); - handlers.register(Box::new(move || { - // write a Ctrl-C packet through the PluginInterface if the plugin is alive and + handlers.register(Box::new(move |action| { + // write a signal packet through the PluginInterface if the plugin is alive and // running if let Some(plugin) = plugin.upgrade() { if let Ok(mutable) = plugin.mutable.lock() { if let Some(ref running) = mutable.running { - let _ = running.interface.ctrlc(); + let _ = running.interface.signal(action); } } } @@ -325,7 +323,7 @@ impl RegisteredPlugin for PersistentPlugin { }; if let Ok(mut mutable) = self.mutable.lock() { - mutable.ctrlc_guard = Some(guard); + mutable.signal_guard = Some(guard); } Ok(()) diff --git a/crates/nu-plugin-protocol/src/lib.rs b/crates/nu-plugin-protocol/src/lib.rs index 00a18beca920..0473b2c499cc 100644 --- a/crates/nu-plugin-protocol/src/lib.rs +++ b/crates/nu-plugin-protocol/src/lib.rs @@ -23,7 +23,8 @@ pub mod test_util; use nu_protocol::{ ast::Operator, engine::Closure, ByteStreamType, Config, DeclId, LabeledError, PipelineData, - PipelineMetadata, PluginMetadata, PluginSignature, ShellError, Span, Spanned, Value, + PipelineMetadata, PluginMetadata, PluginSignature, ShellError, SignalAction, Span, Spanned, + Value, }; use nu_utils::SharedCow; use serde::{Deserialize, Serialize}; @@ -245,8 +246,8 @@ pub enum PluginInput { Drop(StreamId), /// See [`StreamMessage::Ack`]. Ack(StreamId), - /// Signal a ctrlc event - Ctrlc, + /// Relay signals to the plugin + Signal(SignalAction), } impl TryFrom for StreamMessage { diff --git a/crates/nu-plugin/src/plugin/interface/mod.rs b/crates/nu-plugin/src/plugin/interface/mod.rs index daf2d5b78383..fc40b9c732aa 100644 --- a/crates/nu-plugin/src/plugin/interface/mod.rs +++ b/crates/nu-plugin/src/plugin/interface/mod.rs @@ -11,9 +11,9 @@ use nu_plugin_protocol::{ PluginOutput, ProtocolInfo, }; use nu_protocol::{ - engine::{ctrlc, Closure, Sequence}, - Config, DeclId, LabeledError, PipelineData, PluginMetadata, PluginSignature, ShellError, - Signals, Span, Spanned, Value, + engine::{Closure, Sequence}, + Config, DeclId, Handler, HandlerGuard, Handlers, LabeledError, PipelineData, PluginMetadata, + PluginSignature, ShellError, SignalAction, Signals, Span, Spanned, Value, }; use nu_utils::SharedCow; use std::{ @@ -64,10 +64,11 @@ struct EngineInterfaceState { mpsc::Sender<(EngineCallId, mpsc::Sender>)>, /// The synchronized output writer writer: Box>, - // Mirror signals from `EngineState` + /// Mirror signals from `EngineState`. You can make use of this with + /// `engine_interface.signals()` when constructing a Stream that requires signals signals: Signals, - /// Registered Ctrl-C handlers - ctrlc_handlers: ctrlc::Handlers, + /// Registered signal handlers + signal_handlers: Handlers, } impl std::fmt::Debug for EngineInterfaceState { @@ -122,7 +123,7 @@ impl EngineInterfaceManager { engine_call_subscription_sender: subscription_tx, writer: Box::new(writer), signals: Signals::new(Arc::new(AtomicBool::new(false))), - ctrlc_handlers: ctrlc::Handlers::new(), + signal_handlers: Handlers::new(), }), protocol_info_mut, plugin_call_sender: Some(plug_tx), @@ -337,9 +338,12 @@ impl InterfaceManager for EngineInterfaceManager { }); self.send_engine_call_response(id, response) } - PluginInput::Ctrlc => { - self.state.signals.trigger(); - self.state.ctrlc_handlers.run(); + PluginInput::Signal(action) => { + match action { + SignalAction::Interrupt => self.state.signals.trigger(), + SignalAction::Reset => self.state.signals.reset(), + } + self.state.signal_handlers.run(action); Ok(()) } } @@ -521,13 +525,10 @@ impl EngineInterface { self.state.writer.is_stdout() } - /// Register a closure which will be called when the engine receives a Ctrl-C signal. Returns a - /// RAII guard that will keep the closure alive until it is dropped. - pub fn register_ctrlc_handler( - &self, - handler: ctrlc::Handler, - ) -> Result { - self.state.ctrlc_handlers.register(handler) + /// Register a closure which will be called when the engine receives an interrupt signal. + /// Returns a RAII guard that will keep the closure alive until it is dropped. + pub fn register_signal_handler(&self, handler: Handler) -> Result { + self.state.signal_handlers.register(handler) } /// Get the full shell configuration from the engine. As this is quite a large object, it is diff --git a/crates/nu-protocol/src/engine/engine_state.rs b/crates/nu-protocol/src/engine/engine_state.rs index bd049bbb2de5..405b702c1223 100644 --- a/crates/nu-protocol/src/engine/engine_state.rs +++ b/crates/nu-protocol/src/engine/engine_state.rs @@ -2,14 +2,14 @@ use crate::{ ast::Block, debugger::{Debugger, NoopDebugger}, engine::{ - ctrlc, usage::{build_usage, Usage}, CachedFile, Command, CommandType, EnvVars, OverlayFrame, ScopeFrame, Stack, StateDelta, Variable, Visibility, DEFAULT_OVERLAY_NAME, }, eval_const::create_nu_constant, - BlockId, Category, Config, DeclId, FileId, GetSpan, HistoryConfig, Module, ModuleId, OverlayId, - ShellError, Signals, Signature, Span, SpanId, Type, Value, VarId, VirtualPathId, + BlockId, Category, Config, DeclId, FileId, GetSpan, Handlers, HistoryConfig, Module, ModuleId, + OverlayId, ShellError, SignalAction, Signals, Signature, Span, SpanId, Type, Value, VarId, + VirtualPathId, }; use fancy_regex::Regex; use lru::LruCache; @@ -86,8 +86,8 @@ pub struct EngineState { pub spans: Vec, usage: Usage, pub scope: ScopeFrame, - pub ctrlc_handlers: Option, signals: Signals, + pub signal_handlers: Option, pub env_vars: Arc, pub previous_env_vars: Arc>, pub config: Arc, @@ -147,7 +147,7 @@ impl EngineState { 0, false, ), - ctrlc_handlers: None, + signal_handlers: None, signals: Signals::empty(), env_vars: Arc::new( [(DEFAULT_OVERLAY_NAME.to_string(), HashMap::new())] @@ -186,7 +186,10 @@ impl EngineState { } pub fn reset_signals(&mut self) { - self.signals.reset() + self.signals.reset(); + if let Some(ref handlers) = self.signal_handlers { + handlers.run(SignalAction::Reset); + } } pub fn set_signals(&mut self, signals: Signals) { @@ -272,9 +275,9 @@ impl EngineState { #[cfg(feature = "plugin")] if !delta.plugins.is_empty() { for plugin in std::mem::take(&mut delta.plugins) { - // Connect plugins to the ctrlc handlers - if let Some(handlers) = &self.ctrlc_handlers { - plugin.clone().configure_ctrlc_handler(handlers)?; + // Connect plugins to the signal handlers + if let Some(handlers) = &self.signal_handlers { + plugin.clone().configure_signal_handler(handlers)?; } // Replace plugins that overlap in identity. diff --git a/crates/nu-protocol/src/engine/mod.rs b/crates/nu-protocol/src/engine/mod.rs index 24e4b1509433..0cf45217eae2 100644 --- a/crates/nu-protocol/src/engine/mod.rs +++ b/crates/nu-protocol/src/engine/mod.rs @@ -34,5 +34,3 @@ pub use stack_out_dest::*; pub use state_delta::*; pub use state_working_set::*; pub use variable::*; - -pub mod ctrlc; diff --git a/crates/nu-protocol/src/engine/ctrlc.rs b/crates/nu-protocol/src/pipeline/handlers.rs similarity index 83% rename from crates/nu-protocol/src/engine/ctrlc.rs rename to crates/nu-protocol/src/pipeline/handlers.rs index 11121f7c9c30..3c1b1b820ade 100644 --- a/crates/nu-protocol/src/engine/ctrlc.rs +++ b/crates/nu-protocol/src/pipeline/handlers.rs @@ -1,10 +1,10 @@ use std::fmt::Debug; use std::sync::{Arc, Mutex}; -use crate::{engine::Sequence, ShellError}; +use crate::{engine::Sequence, ShellError, SignalAction}; /// Handler is a closure that can be sent across threads and shared. -pub type Handler = Box; +pub type Handler = Box; /// Manages a collection of handlers. #[derive(Clone)] @@ -23,16 +23,16 @@ impl Debug for Handlers { } } -/// Guard that unregisters a handler when dropped. +/// HandlerGuard that unregisters a handler when dropped. #[derive(Clone)] -pub struct Guard { +pub struct HandlerGuard { /// Unique ID of the handler. id: usize, /// Reference to the handlers list. handlers: Arc>>, } -impl Drop for Guard { +impl Drop for HandlerGuard { /// Drops the `Guard`, removing the associated handler from the list. fn drop(&mut self) { if let Ok(mut handlers) = self.handlers.lock() { @@ -41,7 +41,7 @@ impl Drop for Guard { } } -impl Debug for Guard { +impl Debug for HandlerGuard { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("Guard").field("id", &self.id).finish() } @@ -56,23 +56,23 @@ impl Handlers { /// Registers a new handler and returns an RAII guard which will unregister the handler when /// dropped. - pub fn register(&self, handler: Handler) -> Result { + pub fn register(&self, handler: Handler) -> Result { let id = self.next_id.next()?; if let Ok(mut handlers) = self.handlers.lock() { handlers.push((id, handler)); } - Ok(Guard { + Ok(HandlerGuard { id, handlers: Arc::clone(&self.handlers), }) } /// Runs all registered handlers. - pub fn run(&self) { + pub fn run(&self, action: SignalAction) { if let Ok(handlers) = self.handlers.lock() { for (_, handler) in handlers.iter() { - handler(); + handler(action); } } } @@ -99,14 +99,14 @@ mod tests { let called1_clone = Arc::clone(&called1); let called2_clone = Arc::clone(&called2); - let _guard1 = handlers.register(Box::new(move || { + let _guard1 = handlers.register(Box::new(move |_| { called1_clone.store(true, Ordering::SeqCst); })); - let _guard2 = handlers.register(Box::new(move || { + let _guard2 = handlers.register(Box::new(move |_| { called2_clone.store(true, Ordering::SeqCst); })); - handlers.run(); + handlers.run(SignalAction::Interrupt); assert!(called1.load(Ordering::SeqCst)); assert!(called2.load(Ordering::SeqCst)); @@ -119,7 +119,7 @@ mod tests { let called = Arc::new(AtomicBool::new(false)); let called_clone = Arc::clone(&called); - let guard = handlers.register(Box::new(move || { + let guard = handlers.register(Box::new(move |_| { called_clone.store(true, Ordering::Relaxed); })); @@ -131,7 +131,7 @@ mod tests { // Ensure the handler is removed after dropping the guard assert_eq!(handlers.handlers.lock().unwrap().len(), 0); - handlers.run(); + handlers.run(SignalAction::Interrupt); // Ensure the handler is not called after being dropped assert!(!called.load(Ordering::Relaxed)); diff --git a/crates/nu-protocol/src/pipeline/mod.rs b/crates/nu-protocol/src/pipeline/mod.rs index 525806425dfe..0d0931c39669 100644 --- a/crates/nu-protocol/src/pipeline/mod.rs +++ b/crates/nu-protocol/src/pipeline/mod.rs @@ -1,4 +1,5 @@ pub mod byte_stream; +mod handlers; pub mod list_stream; mod metadata; mod out_dest; @@ -6,6 +7,7 @@ mod pipeline_data; mod signals; pub use byte_stream::*; +pub use handlers::*; pub use list_stream::*; pub use metadata::*; pub use out_dest::*; diff --git a/crates/nu-protocol/src/pipeline/signals.rs b/crates/nu-protocol/src/pipeline/signals.rs index 2057e27fe00a..f712f1e7a83e 100644 --- a/crates/nu-protocol/src/pipeline/signals.rs +++ b/crates/nu-protocol/src/pipeline/signals.rs @@ -4,6 +4,8 @@ use std::sync::{ Arc, }; +use serde::{Deserialize, Serialize}; + /// Used to check for signals to suspend or terminate the execution of Nushell code. /// /// For now, this struct only supports interruption (ctrl+c or SIGINT). @@ -75,9 +77,17 @@ impl Signals { self.signals.is_none() } - pub(crate) fn reset(&self) { + pub fn reset(&self) { if let Some(signals) = &self.signals { signals.store(false, Ordering::Relaxed); } } } + +/// The types of things that can be signaled. It's anticipated this will change as we learn more +/// about how we'd like signals to be handled. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +pub enum SignalAction { + Interrupt, + Reset, +} diff --git a/crates/nu-protocol/src/plugin/registered.rs b/crates/nu-protocol/src/plugin/registered.rs index 8749c24af9cb..7f3906310abc 100644 --- a/crates/nu-protocol/src/plugin/registered.rs +++ b/crates/nu-protocol/src/plugin/registered.rs @@ -1,6 +1,6 @@ use std::{any::Any, sync::Arc}; -use crate::{engine::ctrlc, PluginGcConfig, PluginIdentity, PluginMetadata, ShellError}; +use crate::{Handlers, PluginGcConfig, PluginIdentity, PluginMetadata, ShellError}; /// Trait for plugins registered in the [`EngineState`](crate::engine::EngineState). pub trait RegisteredPlugin: Send + Sync { @@ -36,10 +36,7 @@ pub trait RegisteredPlugin: Send + Sync { fn as_any(self: Arc) -> Arc; /// Give this plugin a chance to register for Ctrl-C signals. - fn configure_ctrlc_handler( - self: Arc, - _handler: &ctrlc::Handlers, - ) -> Result<(), ShellError> { + fn configure_signal_handler(self: Arc, _handler: &Handlers) -> Result<(), ShellError> { Ok(()) } } diff --git a/crates/nu_plugin_example/src/commands/ctrlc.rs b/crates/nu_plugin_example/src/commands/ctrlc.rs index f9851a451a70..50da9d24daf1 100644 --- a/crates/nu_plugin_example/src/commands/ctrlc.rs +++ b/crates/nu_plugin_example/src/commands/ctrlc.rs @@ -16,7 +16,7 @@ impl PluginCommand for Ctrlc { } fn usage(&self) -> &str { - "Example command that demonstrates registering a ctrl-c handler" + "Example command that demonstrates registering an interrupt signal handler" } fn signature(&self) -> Signature { @@ -35,12 +35,12 @@ impl PluginCommand for Ctrlc { _input: PipelineData, ) -> Result { let (sender, receiver) = mpsc::channel::<()>(); - let _guard = engine.register_ctrlc_handler(Box::new(move || { + let _guard = engine.register_signal_handler(Box::new(move |_| { let _ = sender.send(()); })); eprintln!("interrupt status: {:?}", engine.signals().interrupted()); - eprintln!("waiting for ctrl-c signal..."); + eprintln!("waiting for interrupt signal..."); receiver.recv().expect("handler went away"); eprintln!("interrupt status: {:?}", engine.signals().interrupted()); eprintln!("peace."); diff --git a/src/signals.rs b/src/signals.rs index 857f639843bc..ca22eef255e3 100644 --- a/src/signals.rs +++ b/src/signals.rs @@ -1,7 +1,4 @@ -use nu_protocol::{ - engine::{ctrlc::Handlers, EngineState}, - Signals, -}; +use nu_protocol::{engine::EngineState, Handlers, SignalAction, Signals}; use std::sync::{ atomic::{AtomicBool, Ordering}, Arc, @@ -11,12 +8,12 @@ pub(crate) fn ctrlc_protection(engine_state: &mut EngineState) { let interrupt = Arc::new(AtomicBool::new(false)); engine_state.set_signals(Signals::new(interrupt.clone())); - let ctrlc_handlers = Handlers::new(); - engine_state.ctrlc_handlers = Some(ctrlc_handlers.clone()); + let signal_handlers = Handlers::new(); + engine_state.signal_handlers = Some(signal_handlers.clone()); ctrlc::set_handler(move || { interrupt.store(true, Ordering::Relaxed); - ctrlc_handlers.run(); + signal_handlers.run(SignalAction::Interrupt); }) .expect("Error setting Ctrl-C handler"); }