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

Use Open3.capture3 to avoid deadlock between stdout/stderr in subprocess #959

Closed
wants to merge 2 commits into from

Conversation

dpkp
Copy link

@dpkp dpkp commented Jan 7, 2021

While investigating a possible issue with our internal use of wicked_pdf / wkhtmltopdf, I found this issue in the source.

After opening the wkhtmltopdf subprocess we attempt to read stderr, and we later use that data to determine whether the process failed or not. But because stdout is open and we ignore it, there is a possibility that the process may deadlock. This can happen if wkhtmltopdf fills its stdout buffer before writing to stderr, in which case the kernel will pause the wkhtmltopdf process until someone reads the buffered stdout data. Because it is paused, there can be no new data written to stderr, which blocks ruby and would cause deadlock.

See discussion here re: Open3.popen3 https://docs.ruby-lang.org/en/2.0.0/Open3.html#method-i-popen3

This patch replaces popen3 with capture3, which allows Open3 to manage the process pipes and simply returns the full stdout, stderr, and process exit status after the subprocess completes. I've also added the process status to the error message and added a case to cover the possibility that the process exits with an error code but does not print anything to stderr.

I haven't added any test cases, but I believe that existing test coverage should guarantee that the subprocess call still works as expected.

@dpkp
Copy link
Author

dpkp commented Jan 8, 2021

Looks like there are some master tests failing, but also this PR triggers a Metrics/CyclomaticComplexity rubocop issue. Let me know how you'd prefer to manage that.

@unixmonkey
Copy link
Collaborator

This is now in version 2.7.0 of wicked_pdf because this change got pulled into this PR. Thank you for your help!

@unixmonkey unixmonkey closed this Aug 24, 2023
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