Improve fault tolerance of addon delegation requests

[johansenja]

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)

[andyw8]

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.

[johansenja]

Thanks for the feedback, and the suggestion! I think I was perhaps running into something similar to what you described, but I can break it down into a bit more detail:

1/ my server_addon wasn't being registered properly (I was registering it too soon, before the server was ready) 2/ my ruby-lsp addon was sending a delegate message to the server (asking for some info from the server_addon) 3/ the server_addon wasn't found, and nothing was returned back to the main addon (which was then hanging) 4/ no further requests could be made because it was still waiting for a message that wasn't coming

So my intention here was to report the error that the extension wasn't found, and this working for me locally (I could carry on using the extension without it freezing up). I guess this is only really a problem when developing an server addon, but it still took me a minute to figure out what was going on.

Could I please clarify what you mean by the following?

we have no way of knowing which message is a request or a notification Is it not ok to assume that server_addon/delegate is always called via make_request? Similar to how eg association_target_location could be assumed to always be called from make_request (because it's expecting a response). Though maybe, to answer my own question, it's to allow for server_addons to also handle notifications in their own way.

I'll give your suggestion a whirl and see if I can recreate the situation again locally, thanks! 🙏🏻

[johansenja]

Having played around with the above solution locally, I noticed some curious behaviour when trying to log messages from within the ServerAddon class methods - the error message is never logged in the console, and if trying to log a message and return a response, the response isn't read properly by the client.

But it would be easy enough to log a response from the block here

        when "server_addon/delegate"
          server_addon_name = params[:server_addon_name]
          request_name = params[:request_name]

          # 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 their own error handling, so a blanket approach is not appropriate.
          ServerAddon.delegate(server_addon_name, request_name, params.except(:request_name, :server_addon_name))
        end

eg if there was simple & narrow error handling

begin
  ServerAddon.delegate(server_addon_name, request_name, params.except(:request_name, :server_addon_name))
rescue MissingAddonError
  log_message("Attempted to delegate #{request_name} to #{server_addon_name}, but that add-on is not registered", type: 1)
end

[vinistock]

@johansenja just coming back to this. I have an idea of how we can add that logging without risking getting the client/server out of sync.

In the runner client, we need to tell the server if the delegate message is a request or notification (so that it can know whether a response is expected or not). Basically, here and here, we would pass an extra argument message_type: :request or message_type: :notification.

Then, the server can know whether to return an error response or simply log without returning anything in the case that we're trying to delegate to a non existing add-on.

What do you think?