LSP: Fails to apply fixes on quick edit & save

Info

  • OS and language server: MacOS Catalina, LSP-eslint

Repro

File: test.js

const a = 1;
  1. Place cursor after semicolon
  2. Press backspace to delete semicolon and immediately save document

Expected: Semicolon is added by fix-on-save feature of eslint server Actual: Semicolon doesn’t get added. Saving document again does add the semicolon.

Works as expected in VSCode.

(Of course, repro requires semi rule to be enabled. I can provide repo with everything set up if needed.)

Logs

Log from LSP (I’ve modified logging code to also log request/notification params):

LSP:  --> textDocument/didChange
LSP:      {'contentChanges': [{'text': 'const a = 1\n'}], 'textDocument': {'version': 3, 'uri': 'file:///test.js'}}
LSP:  ==> textDocument/willSaveWaitUntil
LSP:      {'reason': 1, 'textDocument': {'uri': 'file:///test.js'}}
LSP: timeout on textDocument/willSaveWaitUntil
LSP: <--  textDocument/publishDiagnostics
LSP:      {'diagnostics': [{'range': {'start': {'line': 0, 'character': 6}, 'end': {'line': 0, 'character': 7}}, 'code': 'no-unused-vars', 'source': 'eslint', 'message': "'a' is assigned a value but never used.", 'severity': 1}, {'range': {'start': {'line': 0, 'character': 11}, 'end': {'line': 0, 'character': 11}}, 'code': 'semi', 'source': 'eslint', 'message': 'Missing semicolon.', 'severity': 1}], 'uri': 'file:///test.js'}
LSP: <--  eslint/status
LSP:      {'state': 1}
LSP: Unhandled notification eslint/status
LSP:      [{'range': {'start': {'line': 0, 'character': 11}, 'end': {'line': 0, 'character': 11}}, 'newText': ';'}]
LSP:  --> textDocument/codeAction
LSP:      {'range': {'start': {'line': 0, 'character': 11}, 'end': {'line': 0, 'character': 11}}, 'context': {'diagnostics': [{'range': {'start': {'line': 0, 'character': 11}, 'end': {'line': 0, 'character': 11}}, 'code': 'semi', 'source': 'eslint', 'message': 'Missing semicolon.', 'severity': 1}]}, 'textDocument': {'uri': 'file:///test.js'}}
LSP:      [{'kind': 'quickfix', 'title': 'Fix this semi problem', 'command': {'title': 'Fix this semi problem', 'command': 'eslint.applySingleFix', 'arguments': ['semi']}}, {'kind': 'quickfix', 'title': 'Disable semi for this line', 'command': {'title': 'Disable semi for this line', 'command': 'eslint.applyDisableLine', 'arguments': ['semi']}}, {'kind': 'quickfix', 'title': 'Disable semi for the entire file', 'command': {'title': 'Disable semi for the entire file', 'command': 'eslint.applyDisableFile', 'arguments': ['semi']}}, {'kind': 'quickfix', 'title': 'Show documentation for semi', 'command': {'title': 'Show documentation for semi', 'command': 'eslint.openRuleDoc', 'arguments': ['semi']}}]
LSP:  --> textDocument/didSave
LSP:      {'textDocument': {'uri': 'file:///test.js'}}

Log from VSCode:

[Trace - 8:01:04 PM] Sending notification 'textDocument/didChange'.
Params: {
    "textDocument": {
        "uri": "file:///test.js",
        "version": 25
    },
    "contentChanges": [
        {
            "text": "const a = 1\n"
        }
    ]
}


[Trace - 8:01:04 PM] Sending request 'textDocument/willSaveWaitUntil - (13)'.
Params: {
    "textDocument": {
        "uri": "file:///test.js"
    },
    "reason": 1
}


[Trace - 8:01:04 PM] Received notification 'textDocument/publishDiagnostics'.
Params: {
    "uri": "file:///test.js",
    "diagnostics": [
        {
            "message": "'a' is assigned a value but never used.",
            "severity": 1,
            "source": "eslint",
            "range": {
                "start": {
                    "line": 0,
                    "character": 6
                },
                "end": {
                    "line": 0,
                    "character": 7
                }
            },
            "code": "no-unused-vars"
        },
        {
            "message": "Missing semicolon.",
            "severity": 1,
            "source": "eslint",
            "range": {
                "start": {
                    "line": 0,
                    "character": 11
                },
                "end": {
                    "line": 0,
                    "character": 11
                }
            },
            "code": "semi"
        }
    ]
}


[Trace - 8:01:04 PM] Received notification 'eslint/status'.
Params: {
    "state": 1
}


[Trace - 8:01:04 PM] Received response 'textDocument/willSaveWaitUntil - (13)' in 18ms.
Result: [
    {
        "range": {
            "start": {
                "line": 0,
                "character": 11
            },
            "end": {
                "line": 0,
                "character": 11
            }
        },
        "newText": ";"
    }
]


[Trace - 8:01:04 PM] Sending notification 'textDocument/didChange'.
Params: {
    "textDocument": {
        "uri": "file:///test.js",
        "version": 26
    },
    "contentChanges": [
        {
            "text": "const a = 1;\n"
        }
    ]
}


[Trace - 8:01:04 PM] Sending notification 'textDocument/didSave'.
Params: {
    "textDocument": {
        "uri": "file:///test.js",
        "version": 26
    }
}


[Trace - 8:01:04 PM] Received notification 'textDocument/publishDiagnostics'.
Params: {
    "uri": "file:///test.js",
    "diagnostics": [
        {
            "message": "'a' is assigned a value but never used.",
            "severity": 1,
            "source": "eslint",
            "range": {
                "start": {
                    "line": 0,
                    "character": 6
                },
                "end": {
                    "line": 0,
                    "character": 7
                }
            },
            "code": "no-unused-vars"
        }
    ]
}


[Trace - 8:01:04 PM] Received notification 'eslint/status'.
Params: {
    "state": 1
}

About this issue

  • Original URL
  • State: closed
  • Created 5 years ago
  • Comments: 24 (24 by maintainers)

Most upvoted comments

I can provide all that (not at the moment though).

But it’s a matter of freezing for 1s when saving vs. instant saving. Saving files normally doesn’t introduce any delay, so the difference is very noticeable when it happens. And it would be a little bit hard to visualize that in a screen recording maybe.

+1
rchl on Dec 9, 2019

I’d like to tackle this problem unless @tomv564 is already working on it

+1
rwols on Nov 20, 2019

It was a great start, thank you for that. And then I’ve tried also using same solution in windows.py and that seems to fix the problem completely!

diff --git a/plugin/core/windows.py b/plugin/core/windows.py                                                                                                                       
index 294017b..913628b 100644
--- a/plugin/core/windows.py
+++ b/plugin/core/windows.py
@@ -22,6 +22,18 @@ except ImportError:
     pass
     Protocol = object  # type: ignore
 
+class sublime:  # type: ignore
+    @classmethod
+    def set_timeout_async(cls, func: 'Callable[[], None]', timeout: float) -> None:
+        from time import sleep
+        from threading import Thread                                                                                                                                              
+
+        def do_it() -> None:
+            func()
+
+        single_run = Thread(target=do_it)
+        single_run.start()
+
 
 class SublimeLike(Protocol):
 
@@ -258,7 +270,7 @@ class WindowDocumentHandler(object):
                     "version": buffer_version                                                                                                                                     
                 }
 
-            self._sublime.set_timeout_async(
+            sublime.set_timeout_async(
                 lambda: self.purge_did_change(buffer_id, buffer_version), 500)
 
     def purge_changes(self, view: ViewLike) -> None:
@@ -532,7 +544,7 @@ class WindowManager(object):
                     self._handle_window_closed()
                 else:
                     # in case the window is invalidated after the last view is closed
-                    self._sublime.set_timeout_async(lambda: self._check_window_closed(), 100)                                                                                     
+                    sublime.set_timeout_async(lambda: self._check_window_closed(), 100)
 
     def _check_window_closed(self) -> None:
         # debug('window {} check window closed closing={}, valid={}'.format(

Of course it’s a hack but shouldn’t be hard to turn it into a clean solution.

+1
rchl on Nov 8, 2019

@rchl could you these changes in plugin/core/rpc.py with eslint? This runs all handlers on the ST3 alternative thread. It seems to work OK on my end. However I don’t use eslint.

diff --git a/plugin/core/rpc.py b/plugin/core/rpc.py
index 1ecbf7b..45f6bfd 100644
--- a/plugin/core/rpc.py
+++ b/plugin/core/rpc.py
@@ -11,6 +11,24 @@ try:
 except ImportError:
     pass
 
+try:
+    import sublime  # type: ignore
+except ImportError:
+
+    class sublime:  # type: ignore
+
+        @classmethod
+        def set_timeout_async(cls, func: 'Callable[[], None]', timeout: float) -> None:
+            from time import sleep
+            from threading import Thread
+
+            def do_it() -> None:
+                func()
+
+            single_run = Thread(target=do_it)
+            single_run.start()
+
+
 from .logging import debug, exception_log
 from .protocol import Request, Notification, Response
 from .types import Settings
@@ -72,7 +90,13 @@ def try_terminate_process(process: 'subprocess.Popen') -> None:
 class Client(object):
     def __init__(self, transport: Transport, settings: Settings) -> None:
         self.transport = transport  # type: Optional[Transport]
-        self.transport.start(self.receive_payload, self.on_transport_closed)
+        self.transport.start(
+            # This needs to run on the alternative ST3 thread, otherwise there's the possibility of a deadlock when
+            # handling a notification from the server while doing a blocking request. This is because whatever Python
+            # thread we're on, all ST3 API calls go through the thread that spawned the Python threads. In our case that
+            # should be the main ST3 thread. We want these handlers to run on the alternate ST3 thread.
+            lambda msg: sublime.set_timeout_async(lambda: self.receive_payload(msg), 0),
+            lambda: sublime.set_timeout_async(self.on_transport_closed, 0))
         self.request_id = 0
         self._response_handlers = {}  # type: Dict[int, Tuple[Optional[Callable], Optional[Callable[[Any], None]]]]
         self._request_handlers = {}  # type: Dict[str, Callable]
@@ -163,6 +187,7 @@ class Client(object):
             self.transport.send(message)
 
     def receive_payload(self, message: str) -> None:
+        # Keep in mind: this method runs on the alternative ST3 thread, not the main ST3 thread.
         payload = None
         try:
             payload = json.loads(message)
@@ -186,6 +211,7 @@ class Client(object):
             exception_log("Error handling server payload", err)
 
     def on_transport_closed(self) -> None:
+        # Keep in mind: this method runs on the alternative ST3 thread, not the main ST3 thread.
         self._error_display_handler("Communication to server closed, exiting")
         # Differentiate between normal exit and server crash?
         if not self.exiting:
+1
rwols on Nov 8, 2019

I wonder if that’s because willSaveWaitUntil blocks the thread while eslint wants to send some diagnostic notifications before responding to willSaveWaitUntil…

+1
rchl on Oct 29, 2019
Read more comments on GitHub
← LSP: Diagnostics get out of sync
LSP: Keep using trigger characters, or start using selectors? →