IndexError when defining invalid catalog path in LSP-lemminx

Question

IndexError when defining invalid catalog path in LSP-lemminx

Closed this issue 4 years ago · 9 comments

Describe the bug

LSP crashes with IndexError, when modifying LSP-lemminx's catalog setting, if the values don't point to valid catalogs.

To Reproduce
Steps to reproduce the behavior:

Install LSP-lemminx
Open _Preferences: LSP-lemmings Settings

Add a new setting "xml.settings.catalog": ["/my/invalid/file/path.xmls"]

// Settings in here override those in "LSP-lemminx/LSP-lemminx.sublime-settings"

{
    "settings": {
        "xml.catalogs": [
            "$storage_path/LSP-lemminx/catalogs/catalog_invalid.xml"
        ]
    }
}

Open a XML document

Note:

If an xml document is open, the error dialog is displayed immediatelly after saving settings.
When restarting ST, lemminx correctly detects the file not to exist and runs normally.

It appears some sort of response to config change seems invalid.

Expected behavior

lemminx should run normally with at most an error pointing to the invalid path.

Screenshots

LSP: starting ['java', '-jar', 'C:\\Apps\\Sublime Text\\Data\\Package Storage\\LSP-lemminx\\org.eclipse.lemminx-0.14.1-uber.jar'] in C:\Data\Sublime\Packages
LSP: lemminx: registering capability: codeActionProvider
LSP: lemminx: registering capability: completionProvider
LSP: lemminx: registering capability: documentHighlightProvider
LSP: lemminx: registering capability: hoverProvider
LSP: lemminx: registering capability: renameProvider
LSP: lemminx: registering capability: definitionProvider
LSP: lemminx: registering capability: typeDefinitionProvider
LSP: lemminx: registering capability: referencesProvider
LSP: lemminx: registering capability: documentFormattingProvider
LSP: lemminx: registering capability: rangeFormattingProvider
LSP: lemminx: registering capability: documentSymbolProvider
LSP: lemminx: registering capability: executeCommandProvider
LSP: lemminx: registering capability: executeCommandProvider
LSP: executeCommandProvider is already registered at executeCommandProvider.id with ID 677aafef-3e0e-47cc-9d5c-fb6175f5f195, overwriting
reloading settings Packages/User/AutoFoldCode.sublime-settings
LSP: lemminx: unregistering capability: executeCommandProvider
LSP: lemminx: unregistering capability: executeCommandProvider
LSP: ['java', '-jar', 'C:\\Apps\\Sublime Text\\Data\\Package Storage\\LSP-lemminx\\org.eclipse.lemminx-0.14.1-uber.jar'] ended
reloading settings Packages/User/AutoFoldCode.sublime-settings
reloading settings Packages/User/LSP-lemminx.sublime-settings
Unable to start lemminx
Traceback (most recent call last):
  File "C:\Apps\Sublime Text\Data\Packages\LSP\plugin\core\windows.py", line 285, in start_async
    transport = create_transport(transport_config, cwd, session)
  File "C:\Apps\Sublime Text\Data\Packages\LSP\plugin\core\transports.py", line 191, in create_transport
    startupinfo = _fixup_startup_args(config.command)
  File "C:\Apps\Sublime Text\Data\Packages\LSP\plugin\core\transports.py", line 243, in _fixup_startup_args
    executable_arg = args[0]
IndexError: list index out of range

LSP: [] ended

lemminx: Nov 27, 2020 06:29:45 org.eclipse.lemminx.extensions.contentmodel.uriresolver.XMLCatalogResolverExtension setCatalogs()
Message: Cannot add XML catalog 'C:\Apps\Sublime Text\Data\Package Storage/LSP-lemminx/catalogs/catalog_invalid.xml' with expand system id 'file:///C:/Apps/Sublime%20Text/Data/Package%20Storage/LSP-lemminx/catalogs/catalog_invalid.xml' and root URI 'file:///C:/Apps/Sublime%20Text/Data/Packages/'.
:: <-  lemminx window/logMessage: {'message': "Nov 27, 2020 06:29:45 org.eclipse.lemminx.extensions.contentmodel.uriresolver.XMLCatalogResolverExtension setCatalogs()\r\nMessage: Cannot add XML catalog 'C:\\Apps\\Sublime Text\\Data\\Package Storage/LSP-lemminx/catalogs/catalog_invalid.xml' with expand system id 'file:///C:/Apps/Sublime%20Text/Data/Package%20Storage/LSP-lemminx/catalogs/catalog_invalid.xml' and root URI 'file:///C:/Apps/Sublime%20Text/Data/Packages/'.", 'type': 1}
:: <-  lemminx window/showMessage: {'message': "Invalid path for setting 'xml.catalogs': 'C:\\Apps\\Sublime Text\\Data\\Package Storage/LSP-lemminx/catalogs/catalog_invalid.xml'", 'type': 1}
:: <-  lemminx textDocument/publishDiagnostics: {'uri': 'file:///C:/Apps/Sublime%20Text/Data/Packages/C%23/Symbol%20List%20-%20Inner%20Function.tmPreferences', 'diagnostics': []}
:: --> lemminx textDocument/documentHighlight(2): {'textDocument': {'uri': 'file:///C:/Apps/Sublime%20Text/Data/Packages/C%23/Symbol%20List%20-%20Inner%20Function.tmPreferences'}, 'position': {'character': 0, 'line': 19}}
:: <<< lemminx 2: []
:: --> lemminx textDocument/codeAction(3): {'textDocument': {'uri': 'file:///C:/Apps/Sublime%20Text/Data/Packages/C%23/Symbol%20List%20-%20Inner%20Function.tmPreferences'}, 'context': {'diagnostics': []}, 'range': {'end': {'character': 0, 'line': 19}, 'start': {'character': 0, 'line': 19}}}
:: <-- lemminx workspace/configuration(14): {'items': [{'scopeUri': 'file:///C:/Apps/Sublime%20Text/Data/Packages/C%23/Symbol%20List%20-%20Inner%20Function.tmPreferences', 'section': 'xml.format.insertSpaces'}, {'scopeUri': 'file:///C:/Apps/Sublime%20Text/Data/Packages/C%23/Symbol%20List%20-%20Inner%20Function.tmPreferences', 'section': 'xml.format.tabSize'}]}
:: >>> lemminx 14: [None, None]
:: <<< lemminx 3: []
:: --> lemminx shutdown(4): None
:: <-- lemminx client/unregisterCapability(15): {'unregisterations': [{'method': 'workspace/executeCommand', 'id': 'b996e047-6f6d-4574-bfb5-d72e5d2938d2'}]}
:: ~~> lemminx 15: {'code': -32602, 'message': 'no registration data found for registration ID b996e047-6f6d-4574-bfb5-d72e5d2938d2'}
:: <-- lemminx client/unregisterCapability(16): {'unregisterations': [{'method': 'workspace/executeCommand', 'id': 'f1b6e08c-5dde-489e-bc74-479a36e1d036'}]}
:: ~~> lemminx 16: {'code': -32602, 'message': 'no registration data found for registration ID f1b6e08c-5dde-489e-bc74-479a36e1d036'}
:: <<< lemminx 4: {}
::  -> lemminx exit: None

Environment (please complete the following information):

OS: Windows 10
Sublime Text version: 4093
LSP version: st4000-exploration branch
Language servers used: LSP-lemminx

Additional context
Add any other context about the problem here. For example, whether you're using a helper
package or your manual server configuration in LSP.sublime-settings. When using
a manual server configuration please include it here if you believe it's applicable.

Answer 1 · 2020-11-27T20:30:22.000Z

I believe this is because of the following:

"If any of the settings change, all language servers are restarted" was recently committed: 67974989c0ad6cf75a64184704b8ee100f04c5c2
In the case of lemminx, the command args are set in the AbstractPlugin.configuration callback.
That classmethod is not invoked when reloading due to a settings change, because it could result in an infinite loop.
The command stays an empty list when the settings change.
_fixup_startup_args expects a list with at least one element.

The fix is to set the startup args in the new on_pre_start callback, which is a bit more obvious on what you can do: https://github.com/sublimelsp/LSP/blob/270cd2dfa5bf25584a123b9c896a7167888f0d78/plugin/core/sessions.py#L478-L493

Although, the signature of this classmethod was changed very recently. I will make a new release soon so you have the latest callback signature (was hoping that it wouldn't be used much :) )

Answer 2 · 2020-11-27T21:01:10.000Z

OK well, I released 1.2.0 which includes the signature of on_pre_start mentioned in my previous comment.

Answer 3 · 2020-11-27T21:49:18.000Z

By the way, these lines are alarming to me:

LSP: lemminx: registering capability: executeCommandProvider
LSP: lemminx: registering capability: executeCommandProvider
LSP: executeCommandProvider is already registered at executeCommandProvider.id with ID 677aafef-3e0e-47cc-9d5c-fb6175f5f195, overwriting
LSP: lemminx: unregistering capability: executeCommandProvider
LSP: lemminx: unregistering capability: executeCommandProvider

The first two lines suggest lemminx is registering executeCommandProvider for two different language IDs (or more generally two different DocumentSelectors). But the third line confirms that is not the case. It registers executeCommandProvider twice for some reason. It then removes those two double registrations. cc @angelozerr

Answer 4 · 2020-11-28T11:12:30.000Z

Refreshing the configuration.command basically works. But re-reading client config or not, the command is a member of ClientConfig and shouldn't get lost. How to be able to restart a server if startup commands are dropped?

I mean, it's ok to have the chance to modify them, but in a standard use case they don't. So LSP should keep it internally without required plugin interaction especially with methods you hope are not used often.

Answer 5 · 2020-11-28T11:17:43.000Z

The first two lines suggest lemminx is registering executeCommandProvider for two different language IDs (or more generally two different DocumentSelectors). But the third line confirms that is not the case. It registers executeCommandProvider twice for some reason. It then removes those two double registrations

@rwols it's a bug from LemMinX I think, could you create an issue in LemMinx issues please. Thanks

Answer 6 · 2020-11-28T12:52:08.000Z

Refreshing the configuration.command basically works. But re-reading client config or not, the command is a member of ClientConfig and shouldn't get lost. How to be able to restart a server if startup commands are dropped?

I mean, it's ok to have the chance to modify them, but in a standard use case they don't. So LSP should keep it internally without required plugin interaction especially with methods you hope are not used often.

That sounds like a bug. LSP should get a fresh Plugin.configuration() when settings are changed.

FYI: #11 also fixes this problem but I would still consider this an LSP bug.

Answer 7 · 2020-11-28T13:01:07.000Z

It's not a bug because it would result in a stack overflow if we called .configuration()

Answer 8 · 2020-11-28T13:34:03.000Z

Then maybe the API is wrong in expecting a sublime.Settings object from .configuration() since it has no way of "freezing" and storing the state received from the plugin. Maybe it should just expect a plain Dict?

With current logic configuration() is not expected to mutate the settings object in any way as that state will be lost on reloading the settings. If that's the case then really the API should be changed to just request the path to the settings file from the plugin and LSP should do the reading of the settings itself. Or .configuration() should be deprecated and a new API added that just asks for the path to the settings file.

Answer 9 · 2020-11-28T15:51:56.000Z

Yeah, I'm still unsure what the best approach is. But I do have some ideas floating around, although they haven't materialized into code yet.