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

Prevent broken pipes leading to client errors #406

Merged
merged 1 commit into from
Jul 9, 2024
Merged

Prevent broken pipes leading to client errors #406

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions lib/ruby_lsp/ruby_lsp_rails/runner_client.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,8 @@ def send_message(request, params = nil)
json = message.to_json

@stdin.write("Content-Length: #{json.length}\r\n\r\n", json)
rescue Errno::EPIPE
# The server connection died
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we print something to stderr here? Otherwise it will seem like it's failing silently.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't do it because it will end up printing too much. Once the server died, every single attempt to communicate with it will fail and print to stderr.

Considering that we don't have information about what caused the server to crash, it will be spamming the user with no actionable steps to take.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel a simple message like "Ruby LSP Rails server crashed and didn't respond" will still help debugging a bit as we'll be sure the chain broke at this exact point. But IMO not a blocker, can add it later if proven needed too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My hypothesis is that if the server crashed we will already have printed something. I'll ship as is and we can add the print if we find it difficult to debug without it.

end

alias_method :send_notification, :send_message
Expand All @@ -193,6 +195,9 @@ def read_response
end

response.fetch(:result)
rescue Errno::EPIPE
# The server connection died
nil
end
end

Expand Down
Loading