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.

[vinistock]

I agree that we might be able to improve debugging and make things clearer for add-on authors, but this approach will lead to more severe bugs.

If we simply always return an error response back to the client, we are guaranteed to cause the request/response cycle to go out of sync.

For example, say you fire a delegate notification with a typo in the add-on name. Notifications don't expect a response back as per the LSP spec. By still returning a response regardless, you are putting content in the STDOUT pipe that will not be read by the client immediately.

Now if you fire a second request, the client will try to read its response and will incorrectly read the error response that belonged to a notification, unrelated to the actual request happening now. Everything goes out of sync and there's no way to re-synchronize.

Since we have no way of knowing which message is a request or a notification, we cannot apply a blanket catchall approach as that will always lead to the out of sync problem.

My suggestion is to still do nothing in terms of execution and responses and simply log that with type error to the output tab.

# Type 1 means error
addon = @server_addons[name]

unless addon
  log_message("Attempted to delegate #{request} to #{name}, but that add-on is not registered. Registered add-ons: #{@server_addons.keys.join(",")}", type: 1)
  return
end

addon.execute(request, params)

[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