Add support for asynchronously handling requests #260
Conversation
gatesn
left a comment
gatesn
left a comment
There was a problem hiding this comment.
Haven't reviewed tests, some nice looking stuff here. One thing about the async is that there's no way (I think?) to mix async and sync hook implementations.
This might end up quite a large refactor to support this...
pyls/message_manager.py
Outdated
| log = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class MessageManager(object): |
pyls/message_manager.py
Outdated
|
|
||
| def get_messages(self): | ||
| """ | ||
| Generator that produces well structured JSON RPC message. |
There was a problem hiding this comment.
Docs start on same line as """ without a space. So """Generator that ...
pyls/message_manager.py
Outdated
| request_str = self._read_message() | ||
|
|
||
| if request_str is None: | ||
| # log.error("failed to read message") |
There was a problem hiding this comment.
Should we break here? Looks like we might be at EOF
| try: | ||
| try: | ||
| message_blob = json.loads(request_str) | ||
| message = JSONRPCRequest.from_data(message_blob) |
There was a problem hiding this comment.
Does this support batch requests?
There was a problem hiding this comment.
no, had thought about implementation but forgot to add it. can do it here or in a follow up
| message = JSONRPCRequest.from_data(message_blob) | ||
| except JSONRPCInvalidRequestException: | ||
| # work around where JSONRPC20Reponse expects _id key | ||
| message_blob['_id'] = message_blob['id'] |
There was a problem hiding this comment.
Why are we doing this in an except block? Surely if 'id' in message_blob then we should always just set _id?
There was a problem hiding this comment.
only need to do this to handle to weirdness within JSONRPC20Response constructor, JSONRPCRequest handles things properly
|
|
||
| def _handle_response(self, response): | ||
| try: | ||
| request = self._sent_requests[response._id] |
There was a problem hiding this comment.
Just guard with if response._id not in self._sent_requests - cleaner to read
There was a problem hiding this comment.
its more pythonic to do it this way, EAFP
There was a problem hiding this comment.
Whether you do LBYL or EAFP is only relevant for performance reasons. If this is supposed to fail 99% of the time, do LBYL because exception handingly 99% of the time is much more extensive than the if checking.
Anyways, only optimize after you know what needs optimization, i.e., after benchmarking.
In DonaldKnuth's paper "StructuredProgrammingWithGoToStatements", he wrote: "Programmers waste enormous amounts of time thinking about, or worrying about, the speed of noncritical parts of their programs, and these attempts at efficiency actually have a strong negative impact when debugging and maintenance are considered. We should forget about small efficiencies, say about 97% of the time: premature optimization is the root of all evil. Yet we should not pass up our opportunities in that critical 3%." http://wiki.c2.com/?PrematureOptimization
| PRELOADED_MODULES = get_preferred_submodules() | ||
|
|
||
| def __init__(self, root_uri, lang_server=None): | ||
| def __init__(self, root_uri, rpc_manager=None): |
There was a problem hiding this comment.
Don't like that everything is a manager.
There was a problem hiding this comment.
we can talk about this more offline
| from pyls.python_ls import PythonLanguageServer | ||
| from pyls.workspace import Workspace | ||
| from io import StringIO | ||
| from StringIO import StringIO |
There was a problem hiding this comment.
Revert this:
io.StringIO is a class. It handles Unicode. It reflects the preferred Python 3 library structure.
StringIO.StringIO is a class. It handles strings. It reflects the legacy Python 2 library structure.
| @@ -1,10 +1,11 @@ | |||
| # Copyright 2017 Palantir Technologies, Inc. | |||
| import pytest | |||
| import os | |||
There was a problem hiding this comment.
ya thats something i noticed, didn't seem like linting was actually being run against tests
| # Copyright 2018 Palantir Technologies, Inc. | ||
| import pytest | ||
| import time | ||
| from StringIO import StringIO |
There was a problem hiding this comment.
StringIO isn't the right way, but neither is io.StringIO. it forces you to have to convert all your strings before sending them off, which is totally unnecessary during normal execution
There was a problem hiding this comment.
This will not run on Python 3 and this server is compatible is both Python 2 and 3. You need to:
import sys
if sys.version_info[0] < 3:
from StringIO import StringIO
else:
from io import StringIO
|
@evandrocoan thanks for looking at this! unfortunately this PR was closed in favour of #261 |
3 participants
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.