Skip to content

Introduce Addon::notify to standardize error handling for add-ons #3383

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ryan-robeson
Copy link

Wraps calls to add-on actions with error handling so that they don't bring down the server.

Motivation

Closes #3315

Implementation

Automated Tests

Yes

Wraps calls to add-on actions with error handling so that they don't
bring down the server.

Fixes: Shopify#3315
Copy link

graphite-app bot commented Apr 9, 2025

How to use the Graphite Merge Queue

Add the label graphite-merge to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

@ryan-robeson

This comment was marked as off-topic.

Copy link
Member

@vinistock vinistock left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! Unfortunately, I don't think this will exactly solve the problem.

Those create_..._listener methods are only factories to hook new listeners into the AST dispatching process. An add-on is more likely to raise or accidentally try to exit during the dispatch of the AST events, which is controlled by Prism::Dispatcher.

Also, we cannot have send in the main portions of the automated requests due to performance reasons (dynamic dispatch is significantly slower).

What do you think about adding another branch to our general rescue for system exit and interrupt?

In that spot, we can catch if any features or add-ons are trying to exit and log a warning with the backtrace. Additionally, this won't mess up the LSP's own exiting mechanisms because we call exit outside of the process_message method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Explore preventing add-ons from exiting the process
2 participants