Skip to content

Explore preventing add-ons from exiting the process #3315

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

Open
vinistock opened this issue Mar 18, 2025 · 3 comments · May be fixed by #3383
Open

Explore preventing add-ons from exiting the process #3315

vinistock opened this issue Mar 18, 2025 · 3 comments · May be fixed by #3383
Labels
enhancement New feature or request good-first-issue Good for newcomers help-wanted Extra attention is needed pinned This issue or pull request is pinned and won't be marked as stale server This pull request should be included in the server gem's release notes

Comments

@vinistock
Copy link
Member

Currently, if an add-on executes exit, the Ruby LSP process will die and cause the editor connection to crash.

I've been wondering if we can prevent that from happening. My idea would be to capture the original exit method in a constant (so that we can ourselves use it when shutting down) and then override it to no-op.

Something like this:

ORIGINAL_EXIT = Kernel.method(:exit)

module Kernel
  def exit
    # do nothing
  end
end

# when shutting down
ORIGINAL_EXIT.call
@vinistock vinistock added enhancement New feature or request server This pull request should be included in the server gem's release notes good-first-issue Good for newcomers pinned This issue or pull request is pinned and won't be marked as stale labels Mar 18, 2025
@ryan-robeson
Copy link

Rather than redefining exit globally, would it be sufficient to rescue from SystemExit at the calls to add-on methods? Or perhaps implement an Addon::notify method? Something like:

# Example usage
# Context: Addon.addons.each do |addon|
# This:
# addon.create_code_lens_listener(@response_builder, uri, dispatcher)
# Becomes:
# Addon.notify(addon, :create_code_lens_listener, @response_builder, uri, dispatcher)
def notify(addon, event, *args)
  addon.send(event, *args) if addon.respond_to?(event)
rescue SystemExit
  puts "Add-on #{addon.name} attempted to exit the process while calling #{event} with args (#{args}). Ignoring..."
end

@vinistock
Copy link
Member Author

It might be, but we need to rescue that error in every API point that add-ons have access to. I never actually tried rescuing SystemExit before, so I'd need to confirm it handles all of the different ways to exit (exit, abort...).

ryan-robeson added a commit to ryan-robeson/ruby-lsp that referenced this issue Apr 9, 2025
Wraps calls to add-on actions with error handling so that they don't
bring down the server.

Fixes: Shopify#3315
@ryan-robeson
Copy link

Cool. I submitted a PR for the Addon::notify solution so you can see what you think. It likely will need changes. Wasn't sure if it should catch StandardError too or the proper way to log messages.

Another alternative would be to wrap Addon instances in a class when they're instantiated in load_addons that would wrap the methods with error handling and delegate to that instance. I'm not sure if that would be an improvement though.

@egiurleo egiurleo added the help-wanted Extra attention is needed label Apr 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good-first-issue Good for newcomers help-wanted Extra attention is needed pinned This issue or pull request is pinned and won't be marked as stale server This pull request should be included in the server gem's release notes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants