Add support for asynchronously handling requests #261
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 3 commits
February 15, 2018 14:18
Contributor
|
@evandrocoan @tomv564 @lgeiger as regular contributors, wanted to get your thoughts on this PR. One notable regression is the lack of batch JSON RPC support, are you aware of this impacting any clients? |
Contributor
Author
|
I didn't want the lack of batch requests to be a blocker, so I went ahead and added support for them |
Contributor
|
I just merged this into my master and started my LSP client: File: python-language-server/pyls/python_ls.py
87: def handle_request(self, method, params):
88: """Provides callables to handle requests or responses to those reqeuests
89:
90: Args:
91: method (str): name of the message
92: params (dict): body of the message
93:
94: Returns:
95: Callable if method is to be handled
96:
97: Raises:
98: KeyError: Handler for method is not found
99: """
100:
101: method_call = 'm_{}'.format(_method_to_string(method))
102: if hasattr(self, method_call):
103: return getattr(self, method_call)(**params)
104: elif self._dispatchers: |
Contributor
Author
|
@evandrocoan are you sure you have latest? https://github.com/palantir/python-language-server/pull/261/files#diff-f68667852a14e9f761f6ebf07ba02fc8R138 would indicate that it can handle extra keyword args |
Contributor
|
Sorry, I though this was on your fork, but it is directly put on the upstream. |
Contributor
|
@ferozco, I noticed you seem to have fixed #250 now, how can I access the language server configuration: File: python-language-server/pyls/python_ls.py
147: def m_initialize(self, processId=None, rootUri=None, rootPath=None, initializationOptions=None, **_kwargs):
148: log.debug('Language server initialized with %s %s %s %s', processId, rootUri, rootPath, initializationOptions)
149: if rootUri is None:
150: rootUri = uris.from_fs_path(rootPath) if rootPath is not None else ''
151:
152: self.config = config.Config(rootUri, initializationOptions or {})
153: self.workspace = Workspace(rootUri, rpc_manager=self.rpc_manager)from the Workspace class constructor? I this would be required to fix #256 Currently on my Workspace constructor I have this: File: python-language-server/pyls/workspace.py
77: def __init__(self, root_uri, rpc_manager=None):
78: self._root_uri = root_uri
79: self._rpc_manager = rpc_manager
80: self._root_uri_scheme = uris.urlparse(self._root_uri)[0]
81: self._root_path = uris.to_fs_path(self._root_uri)
82: self._docs = {}
83:
84: # Whilst incubating, keep private
85: if self._rpc_manager.config.plugin_settings('rope').get('create_folder', False):
86: self.__rope = Project(self._root_path)
87: else:
88: self.__rope = Project(self._root_path, ropefolder=None)
89: self.__rope.prefs.set('extension_modules', self.PRELOADED_MODULES)This way, you can fix #179 once we load the client configurations before the creating the Workspace() object. |
Contributor
Author
|
One could argue that the config should just live within the workspace. In this world the language server just becomes a dumb dispatcher. The workspace would then be responsible for loading plugins etc. thoughts @gatesn? I think something like that is outside of the scope of this PR (which already pretty large) |
Contributor
|
Perhaps we should keep a class diagram somewhere like using https://github.com/plantuml/plantum For now, I used
Putting the The old problem still #256 because would make no difference for #179 as we need to already have loaded the user configuration while instantiating the |
Contributor
|
@ferozco, now I got this from my client: This is the log within this so far:
Even running directly from you branch it does this: |
Contributor
|
LSP does not use batch JSON-RPC requests and I have no immediate plans of implementing them. I think offering async request handling (at least for all the linters) could make a good improvement in responsiveness for some user's configurations. Great initiative! |
Contributor
|
@ferozco, even after merging your fix, the problem still happening: |
Contributor
Author
|
@evandrocoan I wasn't able to find a location where we would be sending responses with a null id. it would be helpful if you could either give me some repro steps or add some debug logic to print out what message is causing this error to occur. |
Contributor
|
@ferozco, I added this: File: Data/Packages/LSP/plugin/core/rpc.py
339: def response_handler(self, response):
340: print( "response:", response )
341: handler_id = int(response.get("id")) # dotty sends strings back :(And got: I would suggest to change |
Contributor
|
@ferozco, after merging these changes, the server is crashing some times. This the last stack track logged:
|
Contributor
Author
|
@evandrocoan I think i've found and resolved the issue around the null Ids. Since none of the handlers have been converted to use the new async functionality, it would be unexpected if my change caused this exception. It appears as if there is an unhandled exception coming from the rope plugin. If people are ok, i'd like to move forward and have this merged in and i'll handle any other issues as they come up |
Contributor
|
@ferozco Thanks for the work 🎉 |
gatesn
reviewed
Feb 20, 2018
Contributor
gatesn
left a comment
gatesn
left a comment
There was a problem hiding this comment.
Lots of little nits, but overall looks awesome.
pyls/json_rpc_server.py
Outdated
| from jsonrpc.jsonrpc2 import JSONRPC20Response, JSONRPC20BatchRequest, JSONRPC20BatchResponse | ||
| from jsonrpc.jsonrpc import JSONRPCRequest | ||
| from jsonrpc.exceptions import ( | ||
| JSONRPCInvalidRequestException, |
Contributor
There was a problem hiding this comment.
No need for multiline import
pyls/json_rpc_server.py
Outdated
| from jsonrpc.exceptions import ( | ||
| JSONRPCInvalidRequestException, | ||
| ) | ||
|
|
pyls/json_rpc_server.py
Outdated
| # we do not send out batch requests so no need to support batch responses | ||
| messages = [JSONRPC20Response(**message_blob)] | ||
| except (KeyError, ValueError): | ||
| log.error("Could not parse message %s", request_str) |
Contributor
There was a problem hiding this comment.
Can you change to log.exception so we don't lose the stack trace.
pyls/json_rpc_server.py
Outdated
| def get_messages(self): | ||
| """Generator that produces well structured JSON RPC message. | ||
| Returns: |
Contributor
There was a problem hiding this comment.
Should be Yields: not Returns:
pyls/python_ls.py
Outdated
| from .json_rpc_server import JSONRPCServer | ||
| from .rpc_manager import JSONRPCManager | ||
| from .workspace import Workspace | ||
| from .rpc_manager import MissingMethodException |
Contributor
There was a problem hiding this comment.
Can you collapse the two imports from .rpc_manager into one?
pyls/rpc_manager.py
Outdated
| output = _make_response(request, error=e.error._data) | ||
| except Exception as e: # pylint: disable=broad-except | ||
| # TODO(forozco): add more descriptive error | ||
| log.error('endpoint exception %s %s %s', e.__class__.__name__, e.args, str(e)) |
Contributor
There was a problem hiding this comment.
Again log.exception here. Perhaps "Asynchronous dispatched method exception`
| try: | ||
| request = self._sent_requests[response._id] | ||
| except KeyError: | ||
| log.error('Received unexpected response %s', response.data) |
Contributor
There was a problem hiding this comment.
I usually prefer an explicit check for these sorts of things if response._id not in self._sent_requests. Particularly in this case where the try/else block makes up the bulk of the function.
pyls/workspace.py
Outdated
| PRELOADED_MODULES = get_preferred_submodules() | ||
|
|
||
| def __init__(self, root_uri, lang_server=None): | ||
| def __init__(self, root_uri, rpc_manager=None): |
test/test_language_server.py
Outdated
| except JSONRPCDispatchException as e: | ||
| assert e.error.code == JSONRPCMethodNotFound.CODE | ||
| else: | ||
| assert False |
Contributor
There was a problem hiding this comment.
Can you add a message here? e.g. assert False, "reason for failure"
tox.ini
Outdated
| pycodestyle pyls | ||
| pyflakes pyls | ||
| pylint pyls test | ||
| pycodestyle pyls test --exclude=test/plugins/.ropeproject,test/.ropeproject |
Contributor
There was a problem hiding this comment.
Can you add these excludes to the [pycodestyle] block above?
added 2 commits
February 20, 2018 11:03
gatesn
approved these changes
Feb 20, 2018
evandrocoan
reviewed
Feb 20, 2018
| output = _make_response(request, error=e.error._data) | ||
| except Exception as e: # pylint: disable=broad-except | ||
| log.error('endpoint exception %s %s %s', e.__class__.__name__, e.args, str(e)) | ||
| log.exception('synchronous method handler exception') |
Contributor
There was a problem hiding this comment.
Can we log the original request here like:
log.exception('synchronous method handler exception for request %s', request)
Contributor
Author
There was a problem hiding this comment.
sure! in a follow up
evandrocoan
reviewed
Feb 20, 2018
| except Exception as e: # pylint: disable=broad-except | ||
| # TODO(forozco): add more descriptive error | ||
| log.error('endpoint exception %s %s %s', e.__class__.__name__, e.args, str(e)) | ||
| log.exception('asynchronous method handler exception') |
Contributor
There was a problem hiding this comment.
log.exception('asynchronous method handler exception for request %s, handler %s '
'and completed_future %s', request, handler, completed_future)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously we had a single thread that would handle and subsequently respond to each message in order. This presented performance problems since long running handlers would cause the server to hang from the perspective of the client. Furthermore, prevented a handler from sending (and receiving) multiple message.
This change allows handlers to be executed either synchronously or asynchronously with the choice being left to the specific hookimpl.