/meteor

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

Spiderable phantomjs doesn't exit cleanly #571

Closed
Charuru opened this Issue Dec 25, 2012 · 4 comments

Comments

Assignees
No one assigned
Labels
Projects
None yet
Milestone
No milestone
3 participants
@Charuru @n1mmy @glasser
@Charuru

Charuru commented Dec 25, 2012

Phantomjs doesn't always exit cleanly. This may have to do with javascript errors? or it might not. Whatever it is it builds up processes until it eats all CPU and the site crashes.
@n1mmy

This comment has been minimized.

Sign in to view
Member

n1mmy commented Jan 8, 2013

Thanks for the report. Confirmed and replicated.

I believe this was caused by e27304c
@glasser

This comment has been minimized.

Sign in to view
Member

glasser commented Jan 9, 2013

Specifically, the cp.kill is worthless: it kills the bash, not the phantomjs. Not sure of the best way to address this. One way is to put the timeout into phantom itself (which is fine as long as there are no infinite loops inside phantom).
@glasser

This comment has been minimized.

Sign in to view
Member

glasser commented Jan 9, 2013

A fix is under review on the spiderable-fix-shell branch.
@glasser

This comment has been minimized.

Sign in to view
Member

glasser commented Jan 9, 2013

Merged to devel in aec2e58.

@glasser glasser closed this Jan 9, 2013

tyrchen pushed a commit to tyrchen/meteor that referenced this issue Jan 9, 2013

@glasser
spiderable: Correctly kill phantomjs after timeout. Fixes meteor#571. 
The previous version only killed the wrapper bash script, not the child. We now
work around the broken (socketpair) stdin presented to Node's child processes by
using a bash heredoc rather than "cat |"; this means we can use exec, which only
gives us one process to clean up.

While we're at it, simplify code by switching from child_process.spawn to
child_process.execFile, which does the work of capturing stdout and setting a
timeout for us automatically.
aec2e58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment