Handle exceptions with cached templates #156
Conversation
|
The failing checks seem to all have to do with language features unsupported by Node <6 in files other than the one this PR touches. |
|
Hi, and thanks for the pull request! Sorry this module went through some transitions and I am not maintaining it. I would like to land this with a test, but there is no test included in the changes and I'm not clear on how to reproduce the issue. I will take a look at adding a test for you, but any help in that regard would be much appreciated! |
|
Hi @agarzola I created a test for your PR (and addressed the merge conflicts), but I cannot push to the branch you have on this PR. If you have the opportunity to follow the instructions at https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork I can push the changes and then this PR can be marked as "merged". If not, that's ok, this PR will just show up as "closed" instead, but I can still land the changes. |
|
Hi, @dougwilson. Thanks for adding tests and addressing the conflicts! I really should have added a test case with this PR. I ticked the Allow edits from maintainers checkbox as requested. Thanks again! |
|
Hi @agarzola sorry, you were just a little too late :( I had to get everything out for a security update so I went ahead and pushed it up without merging your PR, I'm sorry. |
|
Oh, and to clarify, your change here did land as commit 39434ad and you are the commit author, FWIW. So though the PR does not show as merged, it is indeed merged. |
|
@dougwilson Oh, that’s quite alright. I’m just happy the fix is in. Thank you! 😄 |
We ran into an issue where errors thrown by cached templates kill the application in such a way that it cannot recover. The process itself is corrupted in such a way that spawning new workers (we’re using lthe
clustermodule) resulted in those new workers dying immediately. @shawnsi found that this seems to be the case for Node.js in Linux (CentOS), yet not in macOS.After a lot of debugging, we identified
hbsas the root cause, as it does not handle errors stemming from cached templates the way it handles them for newly-loaded templates. We haven’t delved too deeply into why this is a problem in one platform and not others.This PR adds error handling for cached templates similar to the way errors are handled for non-cached templates. We’ve found that errors thrown at render time from a bad template or a template invoked with bad data are handle consistently with this fix, and that the application remains healthy and stable.