Refactored configuration management for LSP module by Achal1607 · Pull Request #8514 · apache/netbeans

Achal1607 · 2025-05-21T10:06:08Z

Currently, configuration requests require a round trip every time the language server needs a value from the client. For example, in TextDocumentServiceImpl, the completion method, which provides auto-complete suggestions, performs a round trip to fetch configuration values like netbeans.javadoc.load.timeout and netbeans.completion.warning.time. Additionally, listening to changes in specific configurations is difficult, as it requires registration in WorkspaceServiceImpl and it is not extensible.

This PR aims to address these challenges by introducing a new class, ClientConfigurationManager, which manages clients along with their configurations. It serves configuration values from a local cache if available, and only performs a request to the client when necessary. It also listens for configuration changes to keep the cache in sync.
There is also a new class ConfigValueCache which takes care of all the business logic of traversing the tree and maintaining configs according to the scopes as well.
Moreover, it allows listeners in the form of BiConsumer to be attached to specific configuration keys, enabling appropriate actions to be taken automatically when those configurations change.

PS: Thanks to @sid-srini for helping in the design.

lahodaj

Overall, seems like a sensible direction to me, with some comments for consideration inline.

@sdedic - any insights?

Thanks.

lahodaj · 2025-06-02T07:03:38Z

java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/Server.java

-                    }
-                    textDocumentService.updateProjectJDKHome(newProjectJDKHomePath);
-                });
+                AtomicReference<FileObject> projectDirectory = new AtomicReference<>(null);


The use of AtomicReference here is probably unnecessary, correct? Can it be simply FileObject projectDirectory? It seems to be assign on one place only.

lahodaj · 2025-06-02T07:05:40Z

...lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/ClientConfigurationManager.java

+        }
+    }
+
+    public void registerConfigCache(NbCodeLanguageClient client, String section) {


I wonder what is the purpose of this registering. Can the ConfigValueCache be simply registered on the first query?

Thought was that if a config doesn't want to be cached by the implementor then it can be skipped. Like lets say if we have editor preferences in the future as a configuration and it becomes a large object, then we may not want to cache the entire object, so it is left upto the implementor what they want to cache and what not.
This was my point of view but if you see value in caching the object on first query then we can change to that approach.

I guess my concern is this: given the registration and the actual config read are on completely different places, it is very easy to imagine that when adding a configuration, only the config read will be added, and not the registration. And given it will all work(?), no one will notice, except it will be slower. And after a little while, no one will be sure anymore if the non-caching was intentional or an oversight.

I wonder if we have a particular setting now or in a foreseeable future that we don't want to cache. And if yes, is there a reason why the read cannot go directly through the API without the cache? (With a clear comment saying something along the lines of "not using the cache, as this setting is too big, and should be fetched anew every time, instead of being cache".)

Okay, so thinking on this lines, would this approach be fine:
When the caller requests for the config we will cache it by default and also expose a method which accepts boolean if the caller doesn't want to cache the value (whatever be the reason be it large object size or something else).
So, then it would be the responsibility of the caller to state explicitly if they don't want to cache the value. Would this approach work?

Yes, that sounds reasonable. Thanks!

lahodaj · 2025-06-02T07:07:38Z

...lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/ClientConfigurationManager.java

+ */
+public class ClientConfigurationManager {
+
+    final WeakHashMap<LanguageClient, ConfigValueCache> clientCaches = new WeakHashMap<>();


This is not necessarily wrong, but I wonder if NbCodeLanguageClient should hold an instance of ClientConfigurationManagement. On the call sites it would then be like:

client.getConfigurationManagement().registerConfigChangeListener("<section>", consumer);

and it seems it could be more clearly non-leaking, and also even easier for to use.

lahodaj · 2025-06-02T07:14:51Z

java/java.lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/ConfigValueCache.java

+                if (consumer != null) {
+                    consumer.accept(section, tree);
+                }
+            } catch (RuntimeException e) {


Catching and throwing away an exception like this is usually wrong. If there's something wrong, and the exception happens, we will never find out, things will just appear broken.

In cases there's a valid reason to continue after a callback fails with an exception, the exception should be logged, so that at least from the log we can find out what happened.

In cases we don't see a valid reason why the callback would fail, I would simply not catch the error.

Any update on this?

Sorry, I missed this change, will push this as well.

Achal1607 · 2025-06-07T09:15:14Z

@lahodaj addressed your review comments, please have a look again whenever it is possible. Thanks.

dbalek

Overall, looks reasonable to me.

lahodaj · 2025-06-16T06:11:36Z

.../java.lsp.server/test/unit/src/org/netbeans/modules/java/lsp/server/protocol/ServerTest.java

    }

-    class LspClient implements LanguageClient {
+class LspClient implements LanguageClient {


Nit: this change seems unnecessary?

lahodaj · 2025-06-16T06:34:38Z

...lsp.server/src/org/netbeans/modules/java/lsp/server/protocol/ClientConfigurationManager.java

+        }
+    }
+
+    public void registerConfigCache(NbCodeLanguageClient client, String section) {


I guess my concern is this: given the registration and the actual config read are on completely different places, it is very easy to imagine that when adding a configuration, only the config read will be added, and not the registration. And given it will all work(?), no one will notice, except it will be slower. And after a little while, no one will be sure anymore if the non-caching was intentional or an oversight.

I wonder if we have a particular setting now or in a foreseeable future that we don't want to cache. And if yes, is there a reason why the read cannot go directly through the API without the cache? (With a clear comment saying something along the lines of "not using the cache, as this setting is too big, and should be fetched anew every time, instead of being cache".)

lahodaj · 2025-06-30T06:22:44Z

Seems sensible to me. Please note that there was a test in java.lsp.server failing - I've restarted the test, so we'll see.

@sdedic - please let us know if you have any comments. Thanks!

lahodaj

Looks reasonable to me.

mbien · 2025-11-06T13:47:43Z

@Achal1607 just a nitpick for future PRs: it is better to describe in the commit msg what the commit changed or why the changes were made. When looking through history or running a git bisect "addressed review comments" is no longer helpful at this point.

Achal1607 · 2025-11-06T13:52:37Z

@Achal1607 just a nitpick for future PRs: it is better to describe in the commit msg what the commit changed or why the changes were made. When looking through history or running a git bisect "addressed review comments" is no longer helpful at this point.

Thank you for the feedback, I’ll make sure to include more descriptive and meaningful commit messages in future PRs.

mbien · 2025-11-06T13:59:20Z

@Achal1607 you can still change it if you want ;)

Achal1607 · 2025-11-06T14:09:46Z

Since it is a very old PR, I don't rember exact details, but will try to modify the commit by reading through the history of this PR.

lahodaj · 2025-11-06T15:42:30Z

@Achal1607 , I think what @mbien means is that it would be cleaner if the commit message just said "Refactored configuration management for LSP module", and didn't include these details:

addressed review comments

addressed some review comments

addressed review comments

fixed unit tests

Achal1607 · 2025-11-07T05:57:38Z

@lahodaj , @mbien pushed again with the updated commit message.

Achal1607 requested review from dbalek, lahodaj and sdedic and removed request for lahodaj May 21, 2025 10:06

Achal1607 added Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) LSP [ci] enable Language Server Protocol tests VSCode Extension labels May 21, 2025

Achal1607 added this to the NB27 milestone May 23, 2025

apache locked and limited conversation to collaborators Jun 2, 2025

apache unlocked this conversation Jun 2, 2025

lahodaj reviewed Jun 2, 2025

View reviewed changes

Achal1607 requested a review from lahodaj June 7, 2025 09:15

dbalek reviewed Jun 12, 2025

View reviewed changes

lahodaj reviewed Jun 16, 2025

View reviewed changes

ebarboni modified the milestones: NB27, NB28 Jul 23, 2025

Achal1607 force-pushed the refactor-configuration branch 2 times, most recently from d3a99c4 to 7766ab4 Compare July 24, 2025 14:51

lahodaj approved these changes Jul 28, 2025

View reviewed changes

ebarboni modified the milestones: NB28, NB29 Oct 15, 2025

Achal1607 force-pushed the refactor-configuration branch from 7766ab4 to 7867b01 Compare November 5, 2025 09:46

Refactored configuration management for LSP module

562e2e9

Achal1607 force-pushed the refactor-configuration branch from 7867b01 to 562e2e9 Compare November 7, 2025 05:56

lahodaj merged commit e6ab901 into apache:master Nov 7, 2025
69 of 70 checks passed

Conversation

Achal1607 commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lahodaj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Achal1607 commented Jun 7, 2025

Uh oh!

dbalek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lahodaj commented Jun 30, 2025

Uh oh!

lahodaj left a comment

Choose a reason for hiding this comment

Uh oh!

mbien commented Nov 6, 2025

Uh oh!

Achal1607 commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mbien commented Nov 6, 2025

Uh oh!

Achal1607 commented Nov 6, 2025

Uh oh!

lahodaj commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Achal1607 commented Nov 7, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Achal1607 commented May 21, 2025 •

edited

Loading

Achal1607 commented Nov 6, 2025 •

edited

Loading

lahodaj commented Nov 6, 2025 •

edited

Loading