Closed lorenyu closed 5 years ago
@NickNaso no rush at all, but tagging you just in case you didn't see this PR.
@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!
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
Incidental change
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](https://user-images.githubusercontent.com/447859/54441232-b26d4600-4712-11e9-9b7b-28846cce21d7.png)
After applying the change to ghostscript4js.cc and rebuilding with![image](https://user-images.githubusercontent.com/447859/54441537-5eaf2c80-4713-11e9-8e26-f8eaf087cb16.png)
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.