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.

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

Remove raise from _ReadHeaders #1674

Closed
wants to merge 1 commit into from

Conversation

sc68cal
Copy link

@sc68cal sc68cal commented Jan 10, 2023

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

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 ycm-core#1673
@sc68cal sc68cal force-pushed the ansible-lang-server-bug branch from 4a98005 to f23090d Compare January 10, 2023 16:51
@puremourning
Copy link
Member

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
Copy link
Author

sc68cal commented Jan 10, 2023

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
Copy link
Member

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.

@sc68cal sc68cal closed this Apr 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants