cquery crash: LanguageClient-neovim #247
Comments
|
I went back 10 commits from 32d1c70 and the problem is still there. So I am guessing its probably not a regression on cquery side. I suspect a regression with LanguageClient-neovim. |
|
This seems to be the same problem as #185. |
|
Can you start cquery with |
|
@jiegec I have the same question as you from #185
My json experience is limited, but shouldn't an optional parameter be optional. Again I also see the spec using 'null' explicitly in other contexts as a valid value (which makes sense to me) eg:
|
|
With
|
|
@vargheseg Yep, in the spec, But we should really think if sometime later we do need to consider the differences between |
|
But if the client is not confirming to spec, wouldn't it better to be explicit about it and complain. If this is an incorrect usage (though I am not sure if it is) i would think it better to fail gracefully than workaround the message which is not well formed. |
|
I think we should report this to your LSP client then. I am writing a wiki for this. Edit: wiki updated: https://github.com/jacobdufault/cquery/wiki/Developer-tips#crashing-in-reflect-when-parsing-json |
|
See autozimu/LanguageClient-neovim#265 for feedback from lsp client author. |
|
Quote from the wiki,
Why is server crashing should be reported to client? Even if the request is not confirming to spec, shouldn't the server complain back to client instead of crash itself? |
|
cquery should gracefully exit instead of crashing. There is some error handling but the reflect system generally doesn't deal with it well. So I'm fine with leaving this issue open as a "cquery shouldn't crash but instead give an appropriate error and exit gracefully" |
|
It shouldn’t exit either. If the request is deemed invalid, it should
return error back to client and continue to handle next request or
notification.
…On Sat, Jan 6, 2018 at 20:00 Jacob Dufault ***@***.***> wrote:
cquery should gracefully exit instead of crashing. There is some error
handling but the reflect system generally doesn't deal with it well. So I'm
fine with leaving this issue open as a "cquery shouldn't crash but instead
give an appropriate error and exit gracefully"
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#247 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABYt70cKOSSfmPagvgGOs6zsKfWjimWIks5tIEFugaJpZM4RVeSa>
.
|
Yea, I agree, but this is the initialize request which may be hard to recover from. @MaskRay is modifying the reflection system atm - one way I can think of to support better error handling is to pass an additional context/meta parameter to reach reflection function, which if function fails is set to true or similar. Then the caller can check if reflection failed/succeeded. |
|
I'm not too familiar with the situation in C++, but just out of curiosity is reflection really necessary in this case? Would it be better to serialize from string to struct directly with existing mature library? like https://github.com/nlohmann/json#arbitrary-types-conversions |
|
That's essentially what the reflection system is, just with a lot less typing. The nlohmann json library is also too slow to replace rapidjson. |
|
The error handling mechanism in cquery definitely should be improved. But I also think it would be great if LanguageClient-neovim could be changed to skip undefined optional properties, rather than emit JSON null. |
There seems to be some discrepancy around field being ommited and null value. And null fields are breaking at least some fragile language servers. - autozimu/LanguageClient-neovim#265 - jacobdufault/cquery#247
…tead of undefined/nothing. See issue #247.
|
The example you gave in the top should now at least not crash cquery. |
cquery: 32d1c70
Facing issue similar to #185. Suspect
method: initializemessage framed by the client. Raising this ticket for feed back if this assumption is correct.(For example, initialize message has
workspace:null. From specs think workspace should be optional but if present should be aWorkspaceClientCapabilitiesobject and not null?)LanguageClient.log
LanguageServer.log
The text was updated successfully, but these errors were encountered: