Add --node-ipc support #2015
Add --node-ipc support #2015
Conversation
|
It looks a bit rough code-quality wise but thanks, it can be polished before merging. High-level question to @rwols, @predragnikolic, do you prefer explicit config option for enabling node-ipc or do it like here where we check for the |
Somehow aliasing _read to conn._read breaks it, even though it's not a class method.
+1 for explicit config option My first suggestion for the client config option name would be: What do you think? |
|
How about "stdio", "tcp" and "node-ipc" (as I imagine there might be other kinds of IPC out there)? Don't think RPC makes sense, since all of them are RPC. |
|
I would think something like |
|
Yeah agreed with the boolean There is further separation in |
|
Thanks for all the comments! I'll rework the processor, but in the meantime, does somebody have access to Windows? The pipe is supposed be to be cross-platform, but we're going off-piste here a bit. Edit: Re |
This comment was marked as resolved.
This comment was marked as resolved.
See sublimelsp/LSP#2015 Disabled by default because stdio works in 99.(9)% of cases, so there's no reason to switch until it's proven to be reliable.
| @@ -136,8 +171,8 @@ def __del__(self) -> None: | |||
|
|
|||
| def _read_loop(self) -> None: | |||
| try: | |||
| while self._reader: | |||
| payload = self._processor.read_data(self._reader) | |||
| while True: | |||
There was a problem hiding this comment.
I tried to Google this and also replicate it in REPL with io.BytesIO, but it seems that streams are always truthy? Or am I taking down a Chesterton's fence here?
There was a problem hiding this comment.
@predragnikolic Do you know anything about this? Do self._reader/self._writer ever become falsey?
There was a problem hiding this comment.
Do you know anything about this?
No, I don't know, I just reverted the old code.
I will let someone with more experience answer this question. :)
I can just guess and say that
maybe the ProcessTransport close method should be modified:
def close(self) -> None:
if not self._closed:
self._send_queue.put_nowait(None)
if self._socket:
self._socket.close()
self._closed = True
+ self._reader.close()
+ self._writer.close()
+. self._reader = None
+. self._writer = NoneThere was a problem hiding this comment.
Don't know about stdio and TCP, but the multiprocessing pipe is garbage-collectable: https://docs.python.org/3/library/multiprocessing.html#multiprocessing.connection.Connection.close
|
Thank you for all the feedback again! Please dogfood these changes before merging. ESLint (in the new mode), TypeScript, Pyright, JSON, HTML, YAML work for me so far, but you never know. |
|
Have you verified that it still works when not using |
|
I am also seeing errors, Errors from ST console
Disabling LSP-file-watcher-chokidar will fix the issue. |
|
Then I would try to make changes here so that it's not a breaking change for chokidar watcher. Maybe possible by making the newly added arguments optional. Watcher is using those imports: https://github.com/sublimelsp/LSP-file-watcher-chokidar/blob/1687cbf784dbad6b2fcaa4b3905c414a35cc9900/watcher.py#L7-L11 |
|
Alternatively, if the former would be too messy, we can update the watcher to use the new API but it should be done in a way that supports both new and old API (for a while, at least). |
|
If @alecmev don't mind I would push the change based on Rafal comment? :) If you mind, feel free to revert the last commit. |
plugin/core/transports.py
Outdated
| def __init__(self, name: str, process: subprocess.Popen, socket: Optional[socket.socket], reader: IO[bytes], | ||
| writer: IO[bytes], stderr: Optional[IO[bytes]], processor: AbstractProcessor[T], | ||
| def __init__(self, name: str, process: subprocess.Popen, socket: Optional[socket.socket], reader: Any, | ||
| writer: Any, stderr: Optional[IO[bytes]], processor: AbstractProcessor[T], |
There was a problem hiding this comment.
It would be good that instead of any I used a better type
There was a problem hiding this comment.
This could be solved with generics, I think, but it would again require downstream changes, since it would change AbstractProcessor's signature.
There was a problem hiding this comment.
I have some local changes to clean up the types but I'm yet to find a solution that is fully properly typed. It's complicated.
In any case, that would be something for another PR.
|
on Windows, LSP-json stops right away: I'll investigate some more. |
|
The |
|
Thank you for testing! Sorry, for some reason I was under impression that Windows was POSIX-compliant, but turns out it was just a subsystem, available only in professional editions, and removed a long time ago too. There is When is LSP switching to 3.8? |
plugin/core/transports.py
Outdated
| # https://github.com/python/cpython/blob/17bf6b4671ec02d80ad29b278639d5307baddeb5/Lib/subprocess.py#L706 | ||
| close_fds = True if sys.version_info >= (3, 8, 0) else subprocess._PLATFORM_DEFAULT_CLOSE_FDS # type: ignore |
There was a problem hiding this comment.
_PLATFORM_DEFAULT_CLOSE_FDS doesn't sound like a public API, judging from the leading underscore
It looks like just omitting that argument in the subprocess constructor would be the way to go?
plugin/core/transports.py
Outdated
| return json.dumps( | ||
| data, | ||
| ensure_ascii=False, | ||
| check_circular=False, | ||
| separators=(',', ':') | ||
| ).encode('utf-8') |
plugin/core/transports.py
Outdated
| _buf = bytearray() | ||
| _lines = 0 | ||
|
|
||
| def write_data(self, connection: multiprocessing.connection._ConnectionBase, data: Dict[str, Any]) -> None: |
There was a problem hiding this comment.
Is there really no way to use public type here? Underscore denotes a private property.
There was a problem hiding this comment.
_ConnectionBase was used because multiprocessing.Pipe() uses that.
class NodeIpcPipe():
def __init__(self) -> None:
parent_connection, child_connection = multiprocessing.Pipe()
self.parent_connection = parent_connection
# type: _ConnectionBase
self.child_connection = child_connection
# type _ConnectionBase
I looked a bit at the multiprocessing.Pipe()
def Pipe(duplex=True):
'''
Returns pair of connection objects at either end of a pipe
'''
if duplex:
s1, s2 = socket.socketpair()
s1.setblocking(True)
s2.setblocking(True)
c1 = Connection(s1.detach())
c2 = Connection(s2.detach())
else:
fd1, fd2 = os.pipe()
c1 = Connection(fd1, writable=False)
c2 = Connection(fd2, readable=False)
return c1, c2
and found that maybe we can use the multiprocessing.connection.Connection as the type.
as Connection extends _ConnectionBase, and it doesn't add any public methods
class Connection(_ConnectionBase):
"""
Connection class based on an arbitrary file descriptor (Unix only), or
a socket handle (Windows).
"""
if _winapi:
def _close(self, _close=_multiprocessing.closesocket):
...
else:
def _close(self, _close=os.close):
...
def _send(self, buf, write=_write):
...
def _recv(self, size, read=_read):
...
def _send_bytes(self, buf):
...
def _recv_bytes(self, maxsize=None):
...
def _poll(self, timeout):
...
plugin/core/transports.py
Outdated
| writer.writelines(("Content-Length: {}\r\n\r\n".format(len(body)).encode('ascii'), body)) | ||
| writer.flush() |
There was a problem hiding this comment.
This is potentially breaking for chokidar watcher. We need to add a flush there. I guess duplicate flush() might be ok to avoid adding too much logic there.
plugin/core/transports.py
Outdated
|
|
||
| def read_data(self, connection: multiprocessing.connection._ConnectionBase) -> Optional[Dict[str, Any]]: | ||
| while self._lines == 0: | ||
| chunk = connection._read(connection.fileno(), 65536) # type: ignore |
There was a problem hiding this comment.
Also private API...
I'm not saying it's unacceptable but is there really no higher level API exposed for that?
There was a problem hiding this comment.
This also bothers me. Perhaps switching to some public mechanism also abstracts away the burden of choosing the buffer size? (Currently set to 65KB)
There was a problem hiding this comment.
Very cool overall. With the latest changes using close_fds it still doesn't want to start on Windows, but I'll investigate more. I tried setting the stdout and stdin subprocess handles directly to the parent and child file descriptors but that doesn't seem to work.
plugin/core/transports.py
Outdated
|
|
||
| class NodeIpcProcessor(AbstractProcessor[Dict[str, Any]]): | ||
| _buf = bytearray() | ||
| _lines = 0 |
There was a problem hiding this comment.
Shouldn't these two properties be part of the class instance, instead of the class? If I have LSP-dockerfile, LSP-stylelint, and LSP-typescript running with node IPC, wouldn't these overwrite each other in unpredictable ways? Maybe I'm not understanding something.
There was a problem hiding this comment.
I believe it should work as expected (in most cases ;)) due to how Python works with those.
When instance of the class reads those properties it looks them up first in the instance and then in the class. When writing it always writes to the instance property (creates it if necessary).
The only issue would be if the instance would read a class property (for example self._buf) and then mutate that property (self._buf.append(...) or whatever methods that supports). Then it would mutate the class property.
So I guess it's still safer to set those on the instance.
plugin/core/transports.py
Outdated
|
|
||
| def read_data(self, connection: multiprocessing.connection._ConnectionBase) -> Optional[Dict[str, Any]]: | ||
| while self._lines == 0: | ||
| chunk = connection._read(connection.fileno(), 65536) # type: ignore |
There was a problem hiding this comment.
65536 looks like a rather large buffer (65KB), have you tried 4KB? What are the performance implications compared to the standard transport?
plugin/core/transports.py
Outdated
| @@ -220,27 +245,37 @@ def _stderr_loop(self) -> None: | |||
|
|
|||
|
|
|||
| # Can be a singleton since it doesn't hold any state. | |||
| json_rpc_processor = JsonRpcProcessor() | |||
| standard_processor = StandardProcessor() | |||
| node_ipc_processor = NodeIpcProcessor() | |||
There was a problem hiding this comment.
Again, I'm unsure whether this can actually be a singleton.
There was a problem hiding this comment.
Oh, right. The standard processor wasn't meant to have any instance properties so NodeIpc shouldn't either.
| tcp_port=s.get("tcp_port"), | ||
| auto_complete_selector=s.get("auto_complete_selector"), | ||
| # Default to True, because an LSP plugin is enabled iff it is enabled as a Sublime package. |
There was a problem hiding this comment.
"iff" is short for "if and only if" :) but keep the change, no one really cares.
See: #1389 |
|
Just to give an update:
|
|
Again, don't have time to investigate right now, but it appears to break LSP-chokidar, even when not using any of this functionality explicitly. No didChangeWatchedFiles notifications trigger anymore. EDIT: Actually that is likely due to what I've already mentioned -- missing flush due to a breaking change. |
|
As much as I would like to get this merged. I would like to ask, |
|
Yep. Least effort is #1389. |
|
I was working on this in https://github.com/sublimelsp/LSP/tree/node-ipc, but still could not get the "named pipes" to work on Windows. If you have any ideas or insights, it would be appreciated. Working on this I started to realize that the "duplex pipes" implementation is really just another socket or kind of file descriptor. So I don't see a theoretical performance benefit over plain stdout/stdin. I guess it may help in avoiding a bug in the eslint language server. What other benefits are there with respect to duplex pipes over stdout/stdin ? |
It comes down to this, IMO:
|
Fix #1612
Lots of room for refactoring and better typing and better errors and tests, but went for the cleanest diff possible. Can be tested with stock
LSP-eslintby changingcommandto["${node_bin}", "${server_path}", "--node-ipc"].