search

ycm-core / ycmd

A code-completion & code-comprehension server
https://ycm-core.github.io/ycmd/
GNU General Public License v3.0
1.7k stars 767 forks source link

Remove raise from _ReadHeaders #1674

Closed sc68cal closed 1 year ago

sc68cal commented 1 year ago

The only caller to this method does not handle exceptions from this method, so instead of crashing, just log the message and continue processing

This is related to #1673

This change is Reviewable

puremourning commented 1 year ago

Thanks for sending a PR, but I'm not convinced this is the right thing to do.

The parser will be in an indeterminate state at this point and "just carry on hoping for the best" doesn't seem like the correct result.

For the record:

The only caller to this method does not handle exceptions from this method

that's not strictly true. Exceptions bubble up to the top of the thread in LanguageServerConnection.run where the connection is cleaned up and terminated when we receive invalid protocol data.

sc68cal commented 1 year ago

See, but at least with this patch applied, I get some feedback from the ansible language server and get it to display some hints about my playbook file and show some suggestions, regardless of how flawed the implementation of the language server is. Without it, ycmd just crashes and I get nothing. I think for durability we should at least make ycmd be a bit more resilient in the face of invalid protocol data instead of just crashing.

I'm not saying my approach is the correct approach to accomplish that goal, but I was just hacking on it to make it not crash and make some progress.

puremourning commented 1 year ago

I think fixing ansible-language-server achieves those goals too.

I admit that in the past I have modified this code to print out the invalid protocol data so that it's at least clear what was received, but most language servers don't just output junk to the console and expect clients to handle it.

By removing this exception, we're entering into untestable/unspecifiable behaviour territory, and masking other potential problems. If you feel like improving the diagnostics when this happens, I'm all for that, but I'm not keen on just "hoping for the best" by eating the exception, on error resume next style.