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

Handle fatal errors in ghostscript #41

Merged
merged 5 commits into from
Mar 17, 2019

Conversation

lorenyu
Copy link
Contributor

@lorenyu lorenyu commented Mar 15, 2019

Context

In ghostscript4js, whenever the gs command returns a nonzero exit code it triggers a fatal unrecoverable error that can't be handled in JavaScript. In particular, the GhostscriptManager class tries to throw a string exception and the GhostscriptWorker (subclass of Napi::AsyncWorker) tries to catch it to set the JavaScript error with SetError to e.what(). While I'm not deeply familiar with Napi, I've found that if we throw a std::runtime_error exception instead and change SetError to set the string of the exception rather than use Napi::String::New, we can properly handle the error in JavaScript.

Changes

  • Add unit test to cover this scenario
  • Change GhostscriptManager to throw string instead of char*
  • Change SetError call to set exception string

Incidental change

  • Fix test to not hardcode ghostscript version to 2017

Testing

I added a test to cover this case. The test works by taking an invalid pdf (test/invalid.pdf) which was created by taking an ordinary text file with some Lorem ipsum text and renaming it to have a .pdf extension. When executing a gs command on that PDF file, note that the test suite crashes without finishing the tests, even though the gs.execute had a catch block.
image

After applying the change to ghostscript4js.cc and rebuilding with node-gyp rebuild, I reran npm test and saw that the tests now complete successfully. Note that when ghostscript crashes it still prints out the fatal error stack which I didn't address in this PR but I think it's still better than throwing an unrecoverable error.
image

@lorenyu lorenyu mentioned this pull request Mar 15, 2019
@lorenyu
Copy link
Contributor Author

lorenyu commented Mar 17, 2019

@NickNaso no rush at all, but tagging you just in case you didn't see this PR.

@NickNaso NickNaso self-requested a review March 17, 2019 19:04
Copy link
Owner

@NickNaso NickNaso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @lorenyu I saw the PR and I tested the code and it seems work good. Well done. Before to approve and merge do you want to add yourself on contributors section in the package.json?

@lorenyu
Copy link
Contributor Author

lorenyu commented Mar 17, 2019

@NickNaso thanks! I went ahead and added myself to the contributors list in package.json. Let me know if there's anything else you need from me!

Copy link
Owner

@NickNaso NickNaso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@NickNaso NickNaso merged commit ee29372 into NickNaso:master Mar 17, 2019
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