Skip to content

Conversation

cablehead
Copy link
Contributor

@cablehead cablehead commented Aug 1, 2024

This PR will close #13501

Description

This PR expands on the relay of signals to running plugin processes. 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. This will only affect plugin authors who have started making use of #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
   ╰────

@cablehead cablehead changed the title WIP: fix: relay Signals reset to plugins fix: relay Signals reset to plugins Aug 1, 2024
@cablehead cablehead marked this pull request as ready for review August 1, 2024 16:57
Copy link
Member

@IanManske IanManske left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I think this looks good. I'll let @devyn confirm.

@devyn
Copy link
Contributor

devyn commented Aug 6, 2024

Makes sense to me.

@devyn devyn merged commit 1cd0544 into nushell:main Aug 6, 2024
13 checks passed
@cablehead cablehead deleted the plugin-ctrlc-reset branch August 6, 2024 13:40
@hustcer hustcer added this to the v0.97.0 milestone Aug 7, 2024
@devyn devyn added the plugin-protocol-change Pull requests that will need documentation of the plugin protocol reference to be updated label Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin-protocol-change Pull requests that will need documentation of the plugin protocol reference to be updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EngineInterface.signals() doesn't reset until the plugin process is restarted
4 participants