Handle incoming notifications/responses during a sync request (#787)

This seems to work for eslint where textDocument/publishDiagnostics are sent during a textDocument/willSaveWatUntil request.

Tested this with the following js file:

```javascript
const a = 1;
```

and the following .eslintrc.yaml:

```yaml
---
env:
  node: true
parserOptions:
  ecmaVersion: 2018
  sourceType: module
extends: 'eslint:recommended'
rules:
  semi: error
```

using the LSP-eslint package to set up the eslint language server.

* Run notifications and responses before running the handler for the blocking request
* Add more test assertions
* Allow handling errors in synchronous requests
  We mirror the behavior in execute_request of send_request
* Handle invalid params by passing them as errors
* Fix tests locking up
* It is not needed to call start_active_views(), it'll happen automatically.

plugin/core/protocol.py

@@ -206,6 +206,9 @@ class ErrorCode:
     RequestCancelled = -32800
     ContentModified = -32801
 
+    # Defined by us
+    Timeout = -40000
+
 
 class Error(Exception):
 

plugin/core/transports.py

@@ -229,7 +229,7 @@ def read_stdout(self) -> None:
                         content_length = 0
                     state = STATE_HEADERS
 
-            except IOError as err:
+            except (AttributeError, IOError) as err:
                 self.close()
                 exception_log("Failure reading stdout", err)
                 state = STATE_EOF

plugin/formatting.py

@@ -5,7 +5,7 @@
 from .core.registry import sessions_for_view
 from .core.sessions import Session
 from .core.settings import client_configs
-from .core.typing import List, Optional
+from .core.typing import Any, List, Optional
 from .core.views import will_save_wait_until, text_document_formatting, text_document_range_formatting
 
 
@@ -45,21 +45,22 @@ def _purge_changes_if_needed(self) -> None:
             self.manager.documents.purge_changes(self.view)
             self._view_maybe_dirty = False
 
+    def _apply_and_purge(self, response: Any) -> None:
+        if response:
+            apply_response_to_view(response, self.view)
+            self._view_maybe_dirty = True
+
     def _will_save_wait_until(self, session: Session) -> None:
         client = client_from_session(session)
         if client:
-            response = client.execute_request(will_save_wait_until(self.view, 1))  # TextDocumentSaveReason.Manual
-            if response:
-                apply_response_to_view(response, self.view)
-                self._view_maybe_dirty = True
+            client.execute_request(will_save_wait_until(self.view, reason=1),  # TextDocumentSaveReason.Manual
+                                   lambda response: self._apply_and_purge(response))
 
     def _format_on_save(self) -> None:
         client = client_from_session(session_for_view(self.view, 'documentFormattingProvider'))
         if client:
-            response = client.execute_request(text_document_formatting(self.view))
-            if response:
-                apply_response_to_view(response, self.view)
-                self._view_maybe_dirty = True
+            client.execute_request(text_document_formatting(self.view),
+                                   lambda response: self._apply_and_purge(response))
 
 
 class LspFormatDocumentCommand(LspTextCommand):

tests/setup.py

@@ -137,11 +137,9 @@ def condition() -> bool:
         self.wm = windows.lookup(window)  # create just this single one for the test
         self.assertEqual(len(self.wm._sessions), 0)
         self.view = window.open_file(filename)
-        if sublime.platform() == "osx":
-            yield 200
+        yield {"condition": lambda: not self.view.is_loading(), "timeout": TIMEOUT_TIME, "period": PERIOD_TIME}
         self.assertTrue(self.wm._configs.syntax_supported(self.view))
         self.init_view_settings()
-        self.wm.start_active_views()  # in case testfile.txt was already opened
         yield lambda: len(self.wm._sessions) > 0
         sessions = self.wm._sessions.get(self.config.name, [])
         self.assertEqual(len(sessions), 1)
@@ -171,6 +169,7 @@ def init_view_settings(self) -> None:
         s("tab_size", 4)
         s("translate_tabs_to_spaces", False)
         s("word_wrap", False)
+        s("lsp_format_on_save", False)
 
     def get_view_event_listener(self, unique_attribute: str) -> 'Optional[ViewEventListener]':
         for listener in view_event_listeners[self.view.id()]:

tests/test_rpc.py

@@ -5,9 +5,10 @@
 from LSP.plugin.core.protocol import Request
 from LSP.plugin.core.rpc import Client
 from LSP.plugin.core.rpc import format_request
+from LSP.plugin.core.rpc import SyncRequestStatus
 from LSP.plugin.core.transports import Transport
 from LSP.plugin.core.types import Settings
-from LSP.plugin.core.typing import List, Tuple, Dict, Any
+from LSP.plugin.core.typing import Any, List, Dict, Tuple
 from test_mocks import MockSettings
 import json
 import unittest
@@ -64,6 +65,67 @@ def test_converts_payload_to_string(self):
         self.assertEqual("{}", format_request(dict()))
 
 
+class SyncRequestStatusTest(unittest.TestCase):
+
+    def test_tiny_state_machine(self):
+        sync = SyncRequestStatus()
+        self.assertTrue(sync.is_idle())
+        self.assertFalse(sync.is_requesting())
+        self.assertFalse(sync.is_ready())
+
+        sync.prepare(1)
+        self.assertFalse(sync.is_idle())
+        self.assertTrue(sync.is_requesting())
+        self.assertFalse(sync.is_ready())
+
+        sync.set(1, {"foo": "bar"})
+        self.assertFalse(sync.is_idle())
+        self.assertFalse(sync.is_requesting())
+        self.assertTrue(sync.is_ready())
+        self.assertFalse(sync.has_error())
+
+        payload = sync.flush()
+        self.assertTrue(sync.is_idle())
+        self.assertFalse(sync.is_requesting())
+        self.assertFalse(sync.is_ready())
+        self.assertDictEqual(payload, {"foo": "bar"})
+
+    def test_error_response(self):
+        sync = SyncRequestStatus()
+        sync.prepare(1)
+
+        sync.set_error(1, {"code": 1243, "message": "everything is broken!"})
+        self.assertFalse(sync.is_idle())
+        self.assertFalse(sync.is_requesting())
+        self.assertTrue(sync.is_ready())
+        self.assertTrue(sync.has_error())
+
+        error = sync.flush_error()
+        self.assertTrue(sync.is_idle())
+        self.assertFalse(sync.is_requesting())
+        self.assertFalse(sync.is_ready())
+        self.assertDictEqual(error, {"code": 1243, "message": "everything is broken!"})
+
+    def test_exception_during_requesting(self):
+        sync = SyncRequestStatus()
+        sync.prepare(1)
+        try:
+            raise RuntimeError("oops")
+            sync.set(1234, {"foo": "bar"})  # never reached
+        except Exception:
+            sync.reset()
+        # sync should be usable again
+        self.assertTrue(sync.is_idle())
+        self.assertFalse(sync.is_requesting())
+        self.assertFalse(sync.is_ready())
+
+    def test_assertion_error(self):
+        sync = SyncRequestStatus()
+        sync.prepare(4321)
+        with self.assertRaises(AssertionError):
+            sync.set(4322, {"foo": "bar"})
+
+
 class ClientTest(unittest.TestCase):
 
     def test_can_create_client(self):
@@ -72,20 +134,27 @@ def test_can_create_client(self):
         self.assertIsNotNone(client)
         self.assertTrue(transport.has_started)
 
-    def test_client_request_response(self):
+    def do_client_request_response(self, method):
         transport = MockTransport(return_empty_dict_result)
         settings = MockSettings()
         client = Client(transport, settings)
         self.assertIsNotNone(client)
         self.assertTrue(transport.has_started)
         req = Request.initialize(dict())
         responses = []
-        client.send_request(req, lambda resp: responses.append(resp))
+        do_request = method.__get__(client, Client)
+        do_request(req, lambda resp: responses.append(resp))
         self.assertGreater(len(responses), 0)
         # Make sure the response handler dict does not grow.
         self.assertEqual(len(client._response_handlers), 0)
 
-    def test_client_request_with_none_response(self):
+    def test_client_request_response_async(self):
+        self.do_client_request_response(Client.send_request)
+
+    def test_client_request_response_sync(self):
+        self.do_client_request_response(Client.execute_request)
+
+    def do_client_request_with_none_response(self, method):
         transport = MockTransport(return_null_result)
         settings = MockSettings()
         client = Client(transport, settings)
@@ -94,31 +163,55 @@ def test_client_request_with_none_response(self):
         req = Request.shutdown()
         responses = []
         errors = []
-        client.send_request(req, lambda resp: responses.append(resp), lambda err: errors.append(err))
+        # https://stackoverflow.com/questions/1015307/python-bind-an-unbound-method
+        do_request = method.__get__(client, Client)
+        do_request(req, lambda resp: responses.append(resp), lambda err: errors.append(err))
         self.assertEqual(len(responses), 1)
         self.assertEqual(len(errors), 0)
 
-    def test_client_should_reject_response_when_both_result_and_error_are_present(self):
+    def test_client_request_with_none_response_async(self):
+        self.do_client_request_with_none_response(Client.send_request)
+
+    def test_client_request_with_none_response_sync(self):
+        self.do_client_request_with_none_response(Client.execute_request)
+
+    def do_client_should_reject_response_when_both_result_and_error_are_present(self, method):
         transport = MockTransport(lambda x: '{"id": 1, "result": {"key": "value"}, "error": {"message": "oops"}}')
         settings = MockSettings()
         client = Client(transport, settings)
         req = Request.initialize(dict())
         responses = []
         errors = []
-        client.send_request(req, lambda resp: responses.append(resp), lambda err: errors.append(err))
+        do_request = method.__get__(client, Client)
+        do_request(req, lambda resp: responses.append(resp), lambda err: errors.append(err))
         self.assertEqual(len(responses), 0)
-        self.assertEqual(len(errors), 0)
+        self.assertEqual(len(errors), 1)
+        self.assertEqual(errors[0]["code"], ErrorCode.InvalidParams)
+
+    def test_client_should_reject_response_when_both_result_and_error_are_present_async(self):
+        self.do_client_should_reject_response_when_both_result_and_error_are_present(Client.send_request)
 
-    def test_client_should_reject_response_when_both_result_and_error_keys_are_not_present(self):
+    def test_client_should_reject_response_when_both_result_and_error_are_present_sync(self):
+        self.do_client_should_reject_response_when_both_result_and_error_are_present(Client.execute_request)
+
+    def do_client_should_reject_response_when_both_result_and_error_keys_are_not_present(self, method):
         transport = MockTransport(lambda x: '{"id": 1}')
         settings = MockSettings()
         client = Client(transport, settings)
         req = Request.initialize(dict())
         responses = []
         errors = []
-        client.send_request(req, lambda resp: responses.append(resp), lambda err: errors.append(err))
+        do_request = method.__get__(client, Client)
+        do_request(req, lambda resp: responses.append(resp), lambda err: errors.append(err))
         self.assertEqual(len(responses), 0)
-        self.assertEqual(len(errors), 0)
+        self.assertEqual(len(errors), 1)
+        self.assertEqual(errors[0]["code"], ErrorCode.InvalidParams)
+
+    def test_client_should_reject_response_when_both_result_and_error_keys_are_not_present_async(self):
+        self.do_client_should_reject_response_when_both_result_and_error_keys_are_not_present(Client.send_request)
+
+    def test_client_should_reject_response_when_both_result_and_error_keys_are_not_present_sync(self):
+        self.do_client_should_reject_response_when_both_result_and_error_keys_are_not_present(Client.execute_request)
 
     def test_client_notification(self):
         transport = MockTransport(notify_pong)
@@ -235,7 +328,7 @@ def always_raise_exception(params: Any, request_id: Any) -> None:
             }
         )
 
-    def test_error_response_handler(self):
+    def do_error_response_handler(self, method):
         transport = MockTransport(return_error)
         settings = MockSettings()
         client = Client(transport, settings)
@@ -244,12 +337,19 @@ def test_error_response_handler(self):
         req = Request.initialize(dict())
         errors = []
         responses = []
-        client.send_request(req, lambda resp: responses.append(resp), lambda err: errors.append(err))
+        do_request = method.__get__(client, Client)
+        do_request(req, lambda resp: responses.append(resp), lambda err: errors.append(err))
         self.assertEqual(len(responses), 0)
         self.assertGreater(len(errors), 0)
         self.assertEqual(len(client._response_handlers), 0)
 
-    def test_error_display_handler(self):
+    def test_error_response_handler_async(self):
+        self.do_error_response_handler(Client.send_request)
+
+    def test_error_response_handler_sync(self):
+        self.do_error_response_handler(Client.execute_request)
+
+    def do_error_display_handler(self, method):
         transport = MockTransport(return_error)
         settings = MockSettings()
         client = Client(transport, settings)
@@ -259,11 +359,18 @@ def test_error_display_handler(self):
         errors = []
         client.set_error_display_handler(lambda err: errors.append(err))
         responses = []
-        client.send_request(req, lambda resp: responses.append(resp))
+        do_request = method.__get__(client, Client)
+        do_request(req, lambda resp: responses.append(resp))
         self.assertEqual(len(responses), 0)
         self.assertGreater(len(errors), 0)
         self.assertEqual(len(client._response_handlers), 0)
 
+    def test_error_display_handler_async(self):
+        self.do_error_display_handler(Client.send_request)
+
+    def test_error_display_handler_sync(self):
+        self.do_error_display_handler(Client.execute_request)
+
     def test_handles_transport_closed_unexpectedly(self):
         set_exception_logging(False)
         transport = MockTransport(raise_error)