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

Unclear error with missing executable #27

Closed
jcoupey opened this issue Apr 26, 2019 · 3 comments · Fixed by #40
Closed

Unclear error with missing executable #27

jcoupey opened this issue Apr 26, 2019 · 3 comments · Fixed by #40
Labels
Milestone

Comments

@jcoupey
Copy link
Contributor

jcoupey commented Apr 26, 2019

Whenever the vroom system call can't be performed (e.g. if the executable is not found in PATH), we get a 400 Bad Request and the response is

{"code":2,"error":"Invalid json."}

It should be 500 with a more explicit message.

@jcoupey jcoupey added the bug label Apr 26, 2019
@jcoupey jcoupey added this to the v0.5.0 milestone Apr 26, 2019
@jcoupey
Copy link
Contributor Author

jcoupey commented Apr 26, 2019

Same when we hit an assert within vroom.

@TimMcCauley
Copy link
Collaborator

I have added an internal error in #29 for missing vroom on the PATH which is 127 - https://github.com/VROOM-Project/vroom-express/blob/feature/%2326-code-formatting/src/index.js#L219

ref: http://tldp.org/LDP/abs/html/exitcodes.html

What do you think?

@jcoupey
Copy link
Contributor Author

jcoupey commented May 14, 2019

The 127 case works when the executable is not found in $PATH.

I just gave a try to a vroom crash due to an assert, and the code is 134 but I don't know how relevant this value is across systems.

Probably the best way to make this more robust is to add a default case in the switch after the 127 one for all other unclear situations? I think this is fine: as long as all error codes from config.vroomErrorCodes are filtered, anything else can be seen as a HTTP_INTERNALERROR_CODE.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants