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

Python langpack crash on receiving invalid JSON #9

Open
pmcq opened this issue Nov 8, 2016 · 4 comments
Open

Python langpack crash on receiving invalid JSON #9

pmcq opened this issue Nov 8, 2016 · 4 comments

Comments

@pmcq
Copy link
Contributor

pmcq commented Nov 8, 2016

This shouldn't ever happen with langserver sending data, but the result would be a confusing error message to the user:
https://github.com/algorithmiaio/langpacks/blob/master/python/template/bin/pipe.py#L28

@pmcq
Copy link
Contributor Author

pmcq commented Nov 8, 2016

Looks like ruby and others might have the same issue:
https://github.com/algorithmiaio/langpacks/blob/master/ruby/template/bin/pipe.rb#L13

@anowell
Copy link
Contributor

anowell commented Nov 8, 2016

I'm confused given that "this shouldn't ever happen". Do you just mean that we should still try-catch these lines for the sake of good practice?

@Argoday
Copy link

Argoday commented Nov 8, 2016

Also note that if JSON parsing is aggressively minimized then this langpack JSON parse is the only place that is absolutely required to perform a JSON parse (all other pieces in the stack can theoretically pass it blind) so if these caught malformed JSON in a standardized way then the extra parsing steps can be removed.

@pmcq
Copy link
Contributor Author

pmcq commented Nov 8, 2016

I mean more that we've tested the langserver and it shouldn't send bad JSON; but I was testing the pipe script directly (outside of langserver) and had issues. But yeah, mostly for good practice, help prevent if there was a bug in langserver too etc. I just wanted to call it out since in Go langpack I'm doing an explicit

if err := json.Unmarshal(input, &r); err != nil {
 // return SystemFailure
}

I don't suppose I have to, but just thought consistency across would be ideal (mostly in that the user would probably just see an process died exit 99 type error)

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

No branches or pull requests

3 participants