Refactored configuration management for LSP module #8514
Conversation
Achal1607
added
Java
| } | ||
| textDocumentService.updateProjectJDKHome(newProjectJDKHomePath); | ||
| }); | ||
| AtomicReference<FileObject> projectDirectory = new AtomicReference<>(null); |
There was a problem hiding this comment.
The use of AtomicReference here is probably unnecessary, correct? Can it be simply FileObject projectDirectory? It seems to be assign on one place only.
| } | ||
| } | ||
|
|
||
| public void registerConfigCache(NbCodeLanguageClient client, String section) { |
There was a problem hiding this comment.
I wonder what is the purpose of this registering. Can the ConfigValueCache be simply registered on the first query?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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".)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yes, that sounds reasonable. Thanks!
| */ | ||
| public class ClientConfigurationManager { | ||
|
|
||
| final WeakHashMap<LanguageClient, ConfigValueCache> clientCaches = new WeakHashMap<>(); |
There was a problem hiding this comment.
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.
| if (consumer != null) { | ||
| consumer.accept(section, tree); | ||
| } | ||
| } catch (RuntimeException e) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sorry, I missed this change, will push this as well.
|
@lahodaj addressed your review comments, please have a look again whenever it is possible. Thanks. |
| @@ -346,7 +346,7 @@ void cancelDiagnostics(AtomicBoolean cancel) { | |||
| } | |||
| } | |||
|
|
|||
| class LspClient implements LanguageClient { | |||
| class LspClient implements LanguageClient { | |||
There was a problem hiding this comment.
Nit: this change seems unnecessary?
| } | ||
| } | ||
|
|
||
| public void registerConfigCache(NbCodeLanguageClient client, String section) { |
There was a problem hiding this comment.
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".)
Currently, configuration requests require a round trip every time the language server needs a value from the client. For example, in
TextDocumentServiceImpl, thecompletionmethod, which provides auto-complete suggestions, performs a round trip to fetch configuration values likenetbeans.javadoc.load.timeoutandnetbeans.completion.warning.time. Additionally, listening to changes in specific configurations is difficult, as it requires registration inWorkspaceServiceImpland 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
ConfigValueCachewhich 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
BiConsumerto 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.