fix: don't drop inbound workspace/didChangeConfiguration notifications

[Davidyz]

In #765 there were discussions about the first file opened in neovim won't have the updated config (workspace.library for dynamically adding libraries) sent in the workspace/configuration response.

I believe this was caused by the init_analysis function being called before the workspace manager has been initialized. This means the workspace manager in the workspace/configuration handler is incorrectly marked as uninitialized for the first file.

This PR address the issue, and when used in neovim and lazydev.nvim, emmylua_ls can now correctly load the dynamically added libraries.

[gemini-code-assist]

Summary of Changes

Hello @Davidyz, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request resolves a significant initialization order bug within the emmylua-analyzer-rust project. Previously, the init_analysis function was invoked prematurely, before the workspace manager had completed its setup, which led to incorrect configuration loading for the first file opened in Neovim, particularly affecting dynamic library definitions. By reordering these operations to ensure the workspace manager is fully initialized before analysis begins, this change guarantees that emmylua_ls correctly processes workspace.library configurations, thereby enhancing its compatibility and functionality within Neovim environments, especially when used with lazydev.nvim.

Highlights

• Initialization Order Fix: The init_analysis function is now called after the workspace manager has been fully initialized, resolving a critical timing issue.
• Configuration Loading: This change fixes a bug where the first file opened in Neovim would not correctly load dynamic library configurations, specifically workspace.library.
• Neovim Compatibility: Ensures emmylua_ls correctly processes dynamically added libraries when integrated with Neovim and lazydev.nvim.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request correctly fixes a race condition during initialization by ensuring init_analysis is called after the workspace_manager is fully initialized. The logic is sound and resolves the described issue. I've added one comment regarding a redundant calculation of file patterns that could be refactored for better efficiency and code clarity. Otherwise, the change is good.

[CppCXY]

This is not the correct way to fix the issue. This approach is essentially a race between different requests reaching the language server. It's entirely possible for workspace/configuration to reach the language server first, and then for workspace_manager to finish initialization afterwards.

[CppCXY]

Actually, I know how to fix it, see here, remove the "workspace/didChangeConfiguration"

emmylua-analyzer-rust/crates/emmylua_ls/src/server/message_processor.rs

Line 35 in 978b683

"workspace/didChangeConfiguration" | "$/cancelRequest" | "initialized"

but I think initializing twice is really stupid.

[CppCXY]

I think the place that should be fixed is here:
https://github.com/EmmyLuaLs/emmylua-analyzer-rust/blob/main/crates/emmylua_ls/src/handlers/initialized/client_config/default_config.rs

I have already proactively requested the configuration from the client, so why hasn't it taken effect?

[Davidyz]

This is not the correct way to fix the issue. This approach is essentially a race between different requests reaching the language server. It's entirely possible for workspace/configuration to reach the language server first, and then for workspace_manager to finish initialization afterwards.

Thanks for the comments, but I'm not totally convinced. In init_analysis, the analysis argument is protected by RwLock to prevent the race condition. The file_diagnostic argument also has internal locks/mutex that should handle the concurrency. I believe this is also why you didn't have to do anything special when calling init_analysis in reload_workspace, which is triggered when there's an incoming workspace/didChangeConfiguration noti.

but I think initializing twice is really stupid.

I agree that we should avoid initialize (that is, the initialize in the LSP lifecycle) twice.

I have already proactively requested the configuration from the client, so why hasn't it taken effect?

This might have been a timing issue. In lazydev.nvim (a neovim plugin that dynamically updates workspace.library), the LSP handlers don't get registered from the beginning, but rather when there are some changes in workspace.library. This won't be done until some parsing has been done with the file and some libraries have been identified. This means, when emmylua_ls requests client config in initialized_handler, the workspace/configuration handler in neovim might not have been updated to correctly respond with the updated workspace.library.

[CppCXY]

Thanks for the comments, but I'm not totally convinced. In init_analysis, the analysis argument is protected by RwLock to prevent the race condition. The file_diagnostic argument also has internal locks/mutex that should handle the concurrency. I believe this is also why you didn't have to do anything special when calling init_analysis in reload_workspace, which is triggered when there's an incoming workspace/didChangeConfiguration noti.

This might be a translation issue. When I mentioned "race," I didn't mean a race condition, but rather that the handling order of these two requests could be random. So it's still possible that when processing workspace/configuration, the workspace_manager's initialized flag hasn't been set yet. The reason why swapping the order currently works is because init_analysis takes quite some time to process. I can accept initializing twice on lazyvim, but I can't accept this change affecting other platforms, because in the game environment there could be tens of thousands of Lua files, and the cost of initializing twice is too high.

[Davidyz]

The reason why swapping the order currently works is because init_analysis takes quite some time to process. I can accept initializing twice on lazyvim, but I can't accept this change affecting other platforms, because in the game environment there could be tens of thousands of Lua files, and the cost of initializing twice is too high.

I take it that when you say "initializing twice", you're referring to the init_analysis function being called twice. There are 2 ways that the init_analysis function can be called:

1. in the initialized_handler. This will, by definition, only be called once at the start of the LSP lifecycle.
2. In the handler of a client-to-server notification that calls workspace_manager.reload_workspace or workspace_manager.add_update_emmyrc_task.

Since 1 can only occur once, we can conclude that if init_analysis is called more than once in a very short time frame, it's because the client decided to do so. Regardless of whether the workspace/didChangeConfiguration is quietly thrown away, the client can trigger frequent init_analysis calls by sending notifications that trigger the relevant handlers, and the existing code here won't help with that anyways. I also don't think it's a good idea to just quietly throw away notifications for performance. This eventually makes the LSP operate in an outdated (and therefore incorrect) state.

[CppCXY]

This is the correct method but not the appropriate one. This issue is introduced by lazyvim and is not a problem with all editors. If every editor correctly sent workspace/configuration, I could simply wait for its result before starting initialization. However, that's not the case—some editors may not support workspace/configuration. I can't rely on this behavior for my initialization.

Now there is another method: adding a dedicated --editor lazyvim.dev. In this case, workspace/configuration is added to the pending requests, which results in two initializations.

[Davidyz]

This issue is introduced by lazyvim and is not a problem with all editors. If every editor correctly sent workspace/configuration

I think that's unfair. This issue is caused by the workspace manager not fully initialized after the server sends the InitializeResult. This means the server isn't actually "initialized" when it sends the InitializeResult. The LSP specification only mentions that the client should not send other requests/notifications before it receives the InitializeResult, which neovim/lazydev did not violate. There's no way for the clients to tell when the workspace manager is actually initialized and is ready to process the didChangeConfiguration notifications.

some editors may not support workspace/configuration

May I propose another approach: In initialized_handler, we use param.capabilities to determine whether the client supports the workspace/configuration request (maybe also the didChangeConfiguration notification). If the client doesn't support it, we use the original order (init_analysis first, initialize workspace_manager afterwards) because we don't have to worry about the config changes. If the client supports it, we use the order in this PR so that emmylua correctly listens to the configuration updates.

Update: tbh this alternative approach is still not perfect because there can still be incoming didChangeConfiguration notifications between initialize_finish and workspace manager initialization. The most "correct" approach would be to either queue the incoming notification in the on_did_change_configuration handler until the workspace manager initialization finish, or to move the workspace manager initialization to before connection.initialize_finish. Either way, the notification sent after the LSP InitializeResult is sent shouldn't be ignored.

[CppCXY]

I realized it's actually not that complicated, because in practice, other editors don't actively send workspace/configuration requests. The current check for initialized in workspace/configuration is an outdated solution, since I have modified the main_loop implementation to allow pending requests. Therefore, the initialized field in workspace_manager can be removed and allow pending the workspace/configuration logic .

[Davidyz]

@CppCXY I've removed the workspace_initialized flag and the relevant getter/setter. The init_analysis call in initialize_handler still needs to be after workspace_manager.client_config is updated to make the first analysis run with the up-to-date information.

[CppCXY]

I think you can remove workspace/configuration from emmylua-analyzer-rust/crates/emmylua_ls/src/server/message_processor.rs

[Davidyz]

There might still be something wrong with this PR. Now I sometimes don't get the updated workspace library at all (even after opening the second file). From the logs it seems like the server was able to pull the client configs.

[Davidyz]

@CppCXY I'm noticing deadlocks in init_analysis. Specifically, analysis.write().await hangs when it's triggered by reload workspace. The initialized_handler can finish just fine. The deadlock doesn't always occur, so I missed it before the merge.

[Davidyz]

I was just poking around the notification handlers that are triggered at the beginning of a session, and flipping the order of analysis().read() and workspace_manager().read() seems to fix the deadlock (at least the following init_analysis calls are no longer blocked).

[CppCXY]

can you make a pr fix this?

[Davidyz]

I'm still trying to figure out the underlying mechanism of the deadlock. I encountered some (very rare) deadlocks even with the previously mentioned "fix", so it's probably not the actual culprit. I don't want to submit another non-solution.

btw if you want to revert this PR due to the deadlock, I'm ok with it.

[CppCXY]

I'm still trying to figure out the underlying mechanism of the deadlock. I encountered some (very rare) deadlocks even with the previously mentioned "fix", so it's probably not the actual culprit. I don't want to submit another non-solution.

btw if you want to revert this PR due to the deadlock, I'm ok with it.

I think we can simply restore the use of the initialized field, because it was originally designed to solve deadlocks.

[Davidyz]

I think we can simply restore the use of the initialized field, because it was originally designed to solve deadlocks.

I think that'll bring back the "discarding notification" issue because on_did_change_configuration is the only place that you read the initialized flag for doing things. It'll be just like reverting this PR.

[CppCXY]

I try fix this deadlock: b3d565e
please check is this work

[Davidyz]

@CppCXY seems to work fine. Thanks.

btw, the following is what I've come up with about the deadlock issue:

1. when init_analysis is called, it acquires a write lock on analysis, holds the lock and create a progress bar. During the progress bar creation, it sends a request to the client and await for the response.
2. Before the response arrives, a request arrives, and the corresponding handler tries to acquire a lock on analysis while blocking the message loop in the run function in lsp_server.

Since async notification handlers are spawned to run concurrently, they can run in parallel with the message loop in lsp_server, and the 2 paths mentioned above might result in a deadlock.

I think finer control over the progress bar might help solve this issue. The current implementation also doesn't care whether the client supports progress bar capabilities. If it doesn't (or if the user decided to disable it), and therefore doesn't send a reply to the window/workDoneProgress/create request, the create_progress_task function may hang forever.