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

Improve fault tolerance of addon delegation requests #572

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

Conversation

johansenja
Copy link

Currently there is no error handling for server_addon/delegate requests - which I think makes sense on the whole (in line with comment Do not wrap this in error handlers. Server add-ons need to have the flexibility to choose if they want to include a response or not as part of error handling, so a blanket approach is not appropriate.).

However, I think there is just one case where I think a bit of error handling might be quite helpful - namely if the requested extension is not available. Currently, @server_addons[name]&.execute(request, params) swallows any typos or mis-registered servers - but IMO it should be an error with a clear message reported.

With the current delegate_request implementation in runner_client, make_request sends the server_addon/delegate message to the server, and then gets stuck waiting for a response (which isn't going to come, because @server_addons[name]&.execute(request, params) is nil) - if I'm understanding things correctly.

      def delegate_request(server_addon_name:, request_name:, **params)
        make_request(
          "server_addon/delegate",
          server_addon_name: server_addon_name,
          request_name: request_name,
          **params,
        )
      rescue MessageError
        nil
      end

I'd be curious for any other opinions on this, and if it's of interest I can add some test cases (and/or any more polish)

@johansenja johansenja requested a review from a team as a code owner February 20, 2025 13:01
@andyw8
Copy link
Contributor

andyw8 commented Feb 20, 2025

Overall this makes sense to me. But I would skip the DidYouMean checks, as this seems like something that would only happen once during the development of an add-on.

@vinistock should be back next week, so let's hold off for his opinion.

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.

2 participants