-
Notifications
You must be signed in to change notification settings - Fork 186
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix empty command behavior on windows #2378
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -245,17 +245,22 @@ def create_transport(config: TransportConfig, cwd: Optional[str], | |
else: | ||
stdout = subprocess.PIPE | ||
stdin = subprocess.PIPE | ||
startupinfo = _fixup_startup_args(config.command) | ||
sock = None # type: Optional[socket.socket] | ||
process = None # type: Optional[subprocess.Popen] | ||
|
||
def start_subprocess() -> subprocess.Popen: | ||
startupinfo = _fixup_startup_args(config.command) | ||
return _start_subprocess(config.command, stdin, stdout, subprocess.PIPE, startupinfo, config.env, cwd) | ||
|
||
if config.listener_socket: | ||
assert isinstance(config.tcp_port, int) and config.tcp_port > 0 | ||
process, sock, reader, writer = _await_tcp_connection( | ||
config.name, config.tcp_port, config.listener_socket, start_subprocess) | ||
if config.command: | ||
process, sock, reader, writer = _start_client_process_and_await_connection( | ||
config.listener_socket, | ||
start_subprocess | ||
) | ||
else: | ||
sock, reader, writer = _await_client_connection(config.listener_socket) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I decided to split
and call them depending on presence of Just in case, configuration to reproduce this scenario: // Settings in here override those in "LSP/LSP.sublime-settings"
{
"clients": {
"test-lsp": {
"selector": "source.lisp",
"tcp_port": -9257,
"enabled": true
},
},
} Language server startup will fail with timeout error if language server doesn't try to connect to editor port. Previously it would also fail with "list index out of range". I have tested these changes with manually launched language server running in client and server modes. UPD: Tested in all scenarios - with and without command, in client and server mode. Seems to work fine in all cases. |
||
else: | ||
if config.command: | ||
process = start_subprocess() | ||
|
@@ -337,39 +342,40 @@ def _start_subprocess( | |
return process | ||
|
||
|
||
class _SubprocessData: | ||
def __init__(self) -> None: | ||
self.process = None # type: Optional[subprocess.Popen] | ||
|
||
|
||
def _await_tcp_connection( | ||
name: str, | ||
tcp_port: int, | ||
listener_socket: socket.socket, | ||
subprocess_starter: Callable[[], subprocess.Popen] | ||
) -> Tuple[subprocess.Popen, socket.socket, IO[bytes], IO[bytes]]: | ||
|
||
# After we have accepted one client connection, we can close the listener socket. | ||
def _await_client_connection(listener_socket: socket.socket) -> Tuple[socket.socket, IO[bytes], IO[bytes]]: | ||
with closing(listener_socket): | ||
|
||
# We need to be able to start the process while also awaiting a client connection. | ||
def start_in_background(d: _SubprocessData) -> None: | ||
# Sleep for one second, because the listener socket needs to be in the "accept" state before starting the | ||
# subprocess. This is hacky, and will get better when we can use asyncio. | ||
time.sleep(1) | ||
process = subprocess_starter() | ||
d.process = process | ||
|
||
data = _SubprocessData() | ||
thread = threading.Thread(target=lambda: start_in_background(data)) | ||
thread.start() | ||
# Await one client connection (blocking!) | ||
sock, _ = listener_socket.accept() | ||
thread.join() | ||
|
||
reader = sock.makefile('rwb') # type: ignore | ||
writer = reader | ||
assert data.process | ||
return data.process, sock, reader, writer # type: ignore | ||
return sock, reader, writer # type: ignore | ||
|
||
|
||
def _start_client_process_and_await_connection( | ||
listener_socket: socket.socket, | ||
subprocess_starter: Callable[[], subprocess.Popen] | ||
) -> Tuple[subprocess.Popen, socket.socket, IO[bytes], IO[bytes]]: | ||
process = None | ||
|
||
# We need to be able to start the process while also awaiting a client connection. | ||
def start_in_background() -> None: | ||
# Sleep for one second, because the listener socket needs to be in the "accept" state before starting the | ||
# subprocess. This is hacky, and will get better when we can use asyncio. | ||
time.sleep(1) | ||
|
||
nonlocal process | ||
process = subprocess_starter() | ||
|
||
thread = threading.Thread(target=start_in_background) | ||
thread.start() | ||
|
||
sock, reader, writer = _await_client_connection(listener_socket) | ||
|
||
thread.join() | ||
|
||
assert process is not None | ||
return process, sock, reader, writer # type: ignore | ||
|
||
|
||
def _connect_tcp(port: int) -> Optional[socket.socket]: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the bug is caused by the
_fixup_startup_args
functionstart_subprocess
will be called in 2 cases:In this PR the
_fixup_startup_args
will not be called in Case 2 anymore,so the bug in that case will be fixed.
But in Case 1, when
start_subprocess
is called, the_fixup_startup_args
will also be called, and the command will still be None. So we still have a bug there.IMO, a better option would be to fix the
_fixup_startup_args
function:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like something else is wrong with case 1: it doesn't make sense to start a subprocess with empty command. And
_start_subprocess
will fail when first argument is empty:So, even with check added in
_fixup_startup_args
, it will still fail, but in_start_subprocess
.