Skip to content
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.

Sign up for GitHub

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

Jump to bottom

LSP: Pipes get clogged #229

Closed
strager opened this issue Apr 15, 2021 · 7 comments
Closed

LSP: Pipes get clogged #229

strager opened this issue Apr 15, 2021 · 7 comments
Assignees
@strager

Comments

@strager
Copy link
Collaborator

strager commented Apr 15, 2021

In --lsp-server mode, if the client sends a bunch of requests, but does not read from quick-lint-js' stdout, quick-lint-js can hang while responding to the requests, and the client can hang while sending more requests. (This came up when benchmarking, because my LSP client naively made a bunch of requests without reading responses.)
@strager
Copy link
Collaborator Author

strager commented Apr 18, 2021

Users of lsp_pipe_writer already buffer data in memory. I think it should be easy to have lsp_pipe_writer write as much data as possible without blocking, then have pipe_reader::run call poll/whatever on the two FDs to flush lsp_pipe_writer's data.

@strager strager self-assigned this Apr 18, 2021
@strager
Copy link
Collaborator Author

strager commented Apr 18, 2021 •
edited
Loading

Fixing in #237.

@strager strager linked a pull request Apr 18, 2021 that will close this issue
LSP: Prevent deadlock with synchronous client #237
Closed
@strager
Copy link
Collaborator Author

strager commented Apr 19, 2021

I don't see a way to do this efficiently on Windows.

  • Overlapped I/O is disallowed. We don't have control over how the stdout pipe was created, and it was likely created as an anonymous pipe which we can't reopen in overlapped mode.
  • Blocking pipes require always sending data to a thread (if we want to not block). This means a context switch for every message-write, adding latency. (I didn't measure the added latency, though.)
  • Non-blocking pipes don't have a way to wake up a thread when writing won't block. (There's no equivalent to POSIX' poll+POLLOUT.)

@strager
Copy link
Collaborator Author

strager commented Apr 19, 2021

An untested idea:

  • Set the pipe to blocking mode.
  • Call WriteFile with an OVERLAPPED structure.
  • On another thread, every second, interrupt the WriteFile call with CancelIoEx.

This would avoid the deadlock inefficiently, but wouldn't really harm well-written clients.

I think this proposal isn't worth implementing.

@strager
Copy link
Collaborator Author

strager commented Apr 19, 2021

I'm going to punt on this task. Fixing this issue isn't worth the complexity (Linux) or the [theorized] performance hit (Windows). Saved my progress to refs/archive/lsp-deadlock.

@strager strager closed this as completed Apr 19, 2021
@strager
Copy link
Collaborator Author

strager commented Jun 12, 2021

In order to support config file change notifications (coming from the filesystem) in the LSP server, we need to make an event pump anyway. So we may as well make our event pump not have deadlocks.

An untested idea:

  • Set the pipe to blocking mode.
  • Call WriteFile with an OVERLAPPED structure.
  • On another thread, every second, interrupt the WriteFile call with CancelIoEx.

This would avoid the deadlock inefficiently, but wouldn't really harm well-written clients.

I think this proposal isn't worth implementing.

I suspect this plan will fail because overlapped I/O is unsupported (therefore a synchronous "overlapped" WriteFile won't be cancellable by CancelIoEx).

I think we need to have a separate thread dedicated to calling WriteFile.

I think we can make ReadFile asynchronous/overlapped in practice using WaitForMultipleObjects+PeekNamedPipe. But it might be best to create a separate thread anyway, because ReadFile has interesting interactions with consoles and similar weirdness might exist for anonymous pipes.

I think a ReadFile thread doesn't need to context switch. It could acquire a global lock and call our LSP code as soon as ReadFile returns. I think WriteFile does require a context switch, though.

@strager strager reopened this Jun 12, 2021
strager added a commit that referenced this issue Jun 13, 2021
@strager
Unify writev and no-writev code paths in lsp_pipe_writer
9116a2d 
lsp_pipe_writer has two code paths:

* writev: convert the message byte_buffer into a byte_buffer_iovec, and
  use the writev syscall to write data to the pipe
* no-writev: convert the message byte_buffer into a string8, and use the
  write/WriteFile syscall to write data to the pipe

Having two code paths complicates future changes, such as scheduling
WriteFile on a separate thread [1]. Settle on one code path as much as
possible:

* writev: convert the message byte_buffer into a byte_buffer_iovec, and
  use the writev syscall to write data to the pipe
* no-writev: convert the message byte_buffer into a byte_buffer_iovec
  (same code as if writev is supported), and use the write/WriteFile
  syscall to write data to the pipe

This commit possibly harms performance by calling WriteFile more (with
smaller buffers) on Windows. I didn't attempt to measure this
performance difference.

[1]: #229
@strager
Copy link
Collaborator Author

strager commented Jun 16, 2021

Fixed with a separate thread (all platforms) in commit a7c1481.

Switched to poll() on POSIX platforms in commits a6875d5 and 5fdcb45.

@strager strager closed this as completed Jun 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@strager strager

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

LSP: Prevent deadlock with synchronous client quick-lint/quick-lint-js
1 participant
@strager