[GitHub] [netbeans] matthiasblaesing commented on a diff in pull request #4057: ide/lsp.client fonts-colors and threading.

[GitBox <gi...@apache.org>]

matthiasblaesing commented on code in PR #4057:
URL: https://github.com/apache/netbeans/pull/4057#discussion_r862504739


##########
ide/lsp.client/src/org/netbeans/modules/lsp/client/LSPBindings.java:
##########
@@ -441,11 +471,14 @@ public static synchronized Set<LSPBindings> getAllBindings() {
     private final LanguageServer server;
     private final InitializeResult initResult;
     private final Process process;
+    // LSP Server specific request processor
+    public final RequestProcessor worker;
 
     private LSPBindings(LanguageServer server, InitializeResult initResult, Process process) {
         this.server = server;
         this.initResult = initResult;
         this.process = process;
+        this.worker = new RequestProcessor(server.getClass().getName() + "WORKER", 1, false, false); // NOI18N

Review Comment:
   ```suggestion
           this.worker = new RequestProcessor(server.getClass().getName() + ".WORKER", 1, false, false); // NOI18N
   ```
   
   See comment in line 113.



##########
ide/lsp.client/src/org/netbeans/modules/lsp/client/LSPBindings.java:
##########
@@ -463,12 +496,10 @@ public InitializeResult getInitResult() {
 
     @SuppressWarnings("AccessingNonPublicFieldOfAnotherObject")
     public static synchronized void addBackgroundTask(FileObject file, BackgroundTask task) {
-        RequestProcessor.Task req = WORKER.create(() -> {
-            LSPBindings bindings = getBindings(file);
-
-            if (bindings == null)
-                return ;
-
+        LSPBindings bindings = getBindings(file);
+        if (bindings == null)
+            return ;

Review Comment:
   Please use braces (yeah nitpick):
   
   ```suggestion
           if (bindings == null) {
               return ;
           }
   ```



##########
ide/lsp.client/src/org/netbeans/modules/lsp/client/LSPBindings.java:
##########
@@ -108,7 +110,7 @@ public class LSPBindings {
     private static final int LSP_KEEP_ALIVE_MINUTES = 10;
     private static final int INVALID_START_TIME = 1 * 60 * 1000;
     private static final int INVALID_START_MAX_COUNT = 5;
-    private static final RequestProcessor WORKER = new RequestProcessor(LanguageClientImpl.class.getName(), 1, false, false);
+    private static final RequestProcessor IDLE_BINDINGS_WORKER = new RequestProcessor(LanguageClientImpl.class.getName() + "IDLE", 1, false, false); // NOI18N

Review Comment:
   ```suggestion
       private static final RequestProcessor IDLE_BINDINGS_WORKER = new RequestProcessor(LanguageClientImpl.class.getName() + ".IDLE", 1, false, false); // NOI18N
   ```
   
   Nitpick: Have not looked at the names, but getName() returns a dotted name and the last part should IMHO also be separated.



##########
ide/lsp.client/src/org/netbeans/modules/lsp/client/LSPBindings.java:
##########
@@ -568,6 +599,10 @@ public void run() {
             if(! process.isAlive()) {
                 return;
             }
+            LSPBindings bindings = get();
+            if (bindings != null) {
+                bindings.worker.shutdownNow();
+            }

Review Comment:
   I would be surprised if this works. `LSPReference` is a `WeakReference`, so when this is called, the object is already collected and not accessible from java anymore (at least I assume that his makes cleaning via `ReferenceQueue` easier to reason about than `#finalize`). I might be wrong though.



##########
ide/lsp.client/src/org/netbeans/modules/lsp/client/bindings/BreadcrumbsImpl.java:
##########
@@ -332,9 +330,12 @@ public LSPBreadcrumbPanel(SideBarFactory delegate, JTextComponent component) {
         }
 
         private void update() {
-            WORKER.post(() -> {
-                FileObject file = NbEditorUtilities.getFileObject(component.getDocument());
-                LSPBindings bindings = file != null ? LSPBindings.getBindings(file) : null;
+            FileObject file = NbEditorUtilities.getFileObject(component.getDocument());
+            final LSPBindings bindings = file != null ? LSPBindings.getBindings(file) : null;
+            if (bindings == null) {
+                return;
+            }

Review Comment:
   This looks stange. I stumbled over the fact, that `getInitResult` can't block. It just returns the result of the initialization call to the LSP server, but `getBindings` might block as it might need to spin up a server and `update()` might be called from the EDT (it is called from the constructor. So from my POV this needs to be handled by the `IDLE_WORKER` (maybe `COMMON_WORKER` is a better name?



##########
ide/lsp.client/src/org/netbeans/modules/lsp/client/bindings/TextDocumentSyncServerCapabilityHandler.java:
##########
@@ -159,9 +158,8 @@ private void fireEvent(int start, String newText, String oldText) {
 
                         boolean typingModification = DocumentUtilities.isTypingModification(doc);
                         long documentVersion = DocumentUtilities.getDocumentVersion(doc);
-
-                        WORKER.post(() -> {
-                            LSPBindings server = LSPBindings.getBindings(file);
+                        LSPBindings server = LSPBindings.getBindings(file);

Review Comment:
   Same concern as in `BreadcrumbsImpl`.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org

For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists