Use .capture3 not .popen3 for external parsers #60
Conversation
This file contains hidden or 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
|
👍 awesome find. Seems good to me and the code is much cleaner. Do we have a graph that we can watch to measure the effectiveness of this? Maybe the error rate graph? |
In my testing, I believe this will fix the frequent timeouts we've seen happen on specific files. Like other issues we'd been seeing, these were actually non-deterministic, they just happened to *almost* always fail. But I did see them succeed occasionally: this may be partially why I didn't catch this in testing before the prior release, since I ran it against some of the same projects we saw failing. I just got lucky the first time around. What I saw locally was that sometimes the status thread given by `.open3` would be dead before it was asked for its value, and in those cases the call seemed to succeed. If the thread was still alive when asked for its value, it seemed to then hang indefinitely. Looking at the code for [`.capture3`][1], the biggest difference seems to be the use of threads for capturing stdout/stderr. There's even a warning in [stdlib docs about deadlocking from not doing this][2]! So I think the previous implementation was effectively equivalent to a poor, buggy implementation of `.capture3`, and we should just use the real one. It's unclear to me if the older usage of `IO.popen` also suffered from this behavior and we just happened to be swallowing it, or if the pipe buffer behavior is a little different there. [1]: https://github.com/jruby/jruby/blob/3dbb634f2764b24bca9e92ecd3d52d859cc0e847/lib/ruby/stdlib/open3.rb#L258-L274 [2]: http://ruby-doc.org/stdlib-2.2.3/libdoc/open3/rdoc/Open3.html#method-c-popen3
wfleming
force-pushed
the
will/command-line-running
branch
from
2ccab2e to
0982eeb
Compare
|
Looks promising. Cleaner, too! |
wfleming
added a commit
that referenced
this pull request
Dec 28, 2015
Use .capture3 not .popen3 for external parsers
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.
In my testing, I believe this will fix the frequent timeouts we've seen
happen on specific files.
Like other issues we'd been seeing, these were actually
non-deterministic, they just happened to almost always fail. But I did
see them succeed occasionally: this may be partially why I didn't catch
this in testing before the prior release, since I ran it against some of
the same projects we saw failing. I just got lucky the first time
around.
What I saw locally was that sometimes the status thread given by
.open3wouldbe dead before it was asked for its value, and in those cases the call
seemed to succeed. If the thread was still alive when asked for its
value, it seemed to then hang indefinitely.
Looking at the code for
.capture3, the biggest difference seemsto be the use of threads for capturing stdout/stderr. There's even a
warning in stdlib docs about deadlocking from not doing this! So I
think the previous implementation was effectively equivalent to a poor,
buggy implementation of
.capture3, and we should just use the real one. It'sunclear to me if the older usage of
IO.popenalso suffered from thisbehavior and we just happened to be swallowing it, or if the pipe buffer
behavior is a little different there.
I put together this test repo to stress the duplication engine across all languages. It reliably fails on
masterof this repo & succeeds on this branch.Thoughts, @codeclimate/review ?