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

Update CommandImplementation to handle large stdout #1808

Closed
wants to merge 4 commits into from

Conversation

TylerDixon
Copy link
Contributor

@TylerDixon TylerDixon commented Jun 4, 2024

What?

Markup renderers which were using the CommandImplementation to render the markup were hanging whenever large files were provided. So this PR:

  • Updates the CommandImplementation#execute method to handle large stdout and stderr streams from called subprocesses
  • Add a test to ensure that large RST files don't cause the process to hang

Why?

Previously, the implementation using popen3 would hang in the case of large files. Taking a closer look I tried a few things to see what was going on:

  1. In the rest2html program, instead of writing the results of a large RST file to stdout, I processed the file, wrote it to a file, and just wrote a random short string (asdf) to the stdout buffer. This of course lead to the wrong return result, but, no hang!
  2. Returning rest2html back to it's original state, I tried to read the stdout buffer with stdout.readlines before the wait_thr value. This also lead to no hang.

Based on these 2, we can see that the python and ruby sides were causing one another to hang:

  1. The ruby side was waiting for status to be reported with wait_thr.value.success? before reading the stdout buffer
  2. The python side was hanging because the stdout buffer was full, and therefore wasn't exiting to return a status.

The fix

capture3 is a wrapper around popen3 which creates new threads to read in stdout and stderr while waiting for a status to be reported. This means that we don't end up hanging due to a full buffer. The capture3 implementation can be seen in:

https://github.com/ruby/ruby/blob/83f02d42e0a3c39661dc99c049ab9a70ff227d5b/lib/open3.rb#L648-L677

@TylerDixon TylerDixon force-pushed the tylerdixon-fix-cmd-impl branch from 8d6c963 to 5dabbff Compare June 5, 2024 13:37
@TylerDixon TylerDixon changed the title Update CommandImplementation to handle large stdin Update CommandImplementation to handle large stdout Jun 5, 2024
@TylerDixon TylerDixon requested review from nasquasha and maxbeizer June 5, 2024 14:13
@nasquasha
Copy link

oop CI looking real mad

kenyonj added a commit that referenced this pull request Jun 14, 2024
This also removes support for Ruby versions < 3
Brings in changes from #1808

co-authored-by: Tyler Dixon <tylerdixon@github.com>
@kenyonj
Copy link
Contributor

kenyonj commented Jun 14, 2024

closing in favor of #1814

@kenyonj kenyonj closed this Jun 14, 2024
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.

3 participants