Remove Schmooze in favour of built in fork #53
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Clean up node processor class Refactor some duplicate function in grover class Move appropriate grover specs into processor spec Update TravisCI badge to specify branch
scottharvey
approved these changes
May 12, 2020
There was a problem hiding this comment.
94fdf75b-5bea-45d2-ab10-e4e0857b23a9.pdf
Looking good, I generated several PDFs including a classroom with all the features included and everything looks to be working well.
I'm happy with the code changes as well, nice one 👍
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There is a potential process leak style issue where successive calls to Schmooze will result in node processes being started and not being cleaned up. This also appears to have the side effect of the node process preventing regular Chrome from being able to quit due to some sort of shared memory mutex shared with Chromium that the node process is somehow holding on to. The process is not cleaned up due to how Schmooze uses the Ruby garbage collector to clean up the node process, which has been seen to potentially stick around for a considerable amount of time. If enough Grover instances were launched it would likely result in some sort of resource contention.
Given Grover instantiates a new instance of the Schmooze processor for each call, there is really no need for the node process to be kept alive once the document conversion is complete, thus this PR removes Schmooze in favour of a built in fork that is somewhat tailored for the Grover process.