Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Goodbye message to ensure plugins exit when they are no longer needed #12014

Merged
merged 2 commits into from Feb 29, 2024

Conversation

devyn
Copy link
Contributor

@devyn devyn commented Feb 28, 2024

Description

This fixes a race condition where all interfaces to a plugin might have been dropped, but both sides are still expecting input, and the PluginInterfaceManager doesn't get a chance to see that the interfaces have been dropped and stop trying to consume input.

As the manager needs to hold on to a writer, we can't automatically close the stream, but we also can't interrupt it if it's in a waiting to read. So the best solution is to send a message to the plugin that we are no longer going to be sending it any plugin calls, so that it knows that it can exit when it's done.

This race condition is a little bit tricky to trigger as-is, but can be more noticeable when running plugins in a tight loop. If too many plugin processes are spawned at one time, Nushell can start to encounter "too many open files" errors, and not be very useful.

User-Facing Changes

Tests + Formatting

  • 🟢 toolkit fmt
  • 🟢 toolkit clippy
  • 🟢 toolkit test
  • 🟢 toolkit test stdlib

After Submitting

I will need to add Goodbye to the protocol docs

…eded

This fixes a race condition where all interfaces to a plugin might have
been dropped, but both sides are still expecting input, and the
`PluginInterfaceManager` doesn't get a chance to see that the interfaces
have been dropped and stop trying to consume input.

As the manager needs to hold on to a writer, we can't automatically
close the stream, but we also can't interrupt it if it's in a waiting
to read. So the best solution is to send a message to the plugin that
we are no longer going to be sending it any plugin calls, so that it
knows that it can exit when it's done.

This race condition is a little bit tricky to trigger as-is, but can
be more noticeable when running plugins in a tight loop. If too many
plugin processes are spawned at one time, Nushell can start to encounter
"too many open files" errors, and not be very useful.
@fdncred fdncred added the pr:plugins This PR is related to plugins label Feb 29, 2024
@fdncred fdncred merged commit ab08328 into nushell:main Feb 29, 2024
19 checks passed
@fdncred
Copy link
Collaborator

fdncred commented Feb 29, 2024

Thanks

@fdncred fdncred added the pr:bugfix This PR fixes some bug label Feb 29, 2024
@hustcer hustcer added this to the v0.91.0 milestone Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:bugfix This PR fixes some bug pr:plugins This PR is related to plugins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants