Handle all available bash output #193
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
Codecov Report
@@ Coverage Diff @@
## master #193 +/- ##
==========================================
+ Coverage 65.73% 66.45% +0.72%
==========================================
Files 53 54 +1
Lines 3315 3452 +137
==========================================
+ Hits 2179 2294 +115
- Misses 1136 1158 +22
Continue to review full report at Codecov.
|
|
Merged, thanks! |
mbland
added a commit
to mbland/kcov
that referenced
this pull request
Jul 16, 2017
Pull SimonKagstrom#193 (written to resolve SimonKagstrom#192) caused the mbland/go-script-bash Travis build to fail (which clones SimonKagstrom/kcov each time) when kcov would hang after printing the number of Bats test cases on Linux. This change resolves the issue, but I am still working to write a focused test case, both to understand the exact problem and to prevent a regression.
mbland
added a commit
to mbland/kcov
that referenced
this pull request
Jul 16, 2017
Pull SimonKagstrom#193 (written to resolve SimonKagstrom#192) caused the mbland/go-script-bash Travis build to fail. That build clones SimonKagstrom/kcov each time, and after that pull request, kcov began hanging after printing the number of Bats test cases on Linux, e.g.: https://travis-ci.org/mbland/go-script-bash/jobs/246933167 This change resolves the issue by calling `file_readable` on standard output before calling `getline`. However, I am still working to write a focused test case, both to understand the exact problem and to prevent a regression.
mbland
added a commit
to mbland/kcov
that referenced
this pull request
Jul 20, 2017
Per the previous commit, this reproduces the problem caused by SimonKagstrom#193 whereby using `getline` within the `handleStdout` function in `src/engines/bash-engine.cc` would cause kcov to hang indefinitely under certain conditions. Though I don't have a perfect understanding yet, it appears that Bash scripts that run for some amount of time and produce some amount of standard output eventually end up blocking when trying to write. When the parent `kcov` process subsequently blocks in `getline`, the child process running the Bash script doesn't appear to be able to resume, implying some sort of deadlock. At any rate, the new bash_handle_all_output test case reliably reproduces the failure and validates the fix from the previous commit. It required adding a timeout capability to `KcovTestCase.do`. I've also removed `@unittest.expectedFaulure` from `bash_subshell`, as it isn't appear necessary and confirms that the behavior implemented by SimonKagstrom#193 remains intact.
mbland
added a commit
to mbland/kcov
that referenced
this pull request
Jul 20, 2017
Per the previous commit, this reproduces the problem caused by SimonKagstrom#193 whereby multiple calls to `getline` within the `handleStdout` function in `src/engines/bash-engine.cc` would cause kcov to hang indefinitely. The problem was due to the fact that the buffer to which Bash was writing kcov tracing messages was getting full, so Bash would eventually block on `write` calls to the `xtraceFd` (FD 782). After that point, calls to `getline` to read the standard output from Bash would block as well, creating a deadlock between the kcov parent and Bash child processes. Using a build of kcov from before the previous commit, running an `strace` command similar to the following revealed that the xtraceFd (FD 782) buffer was getting full: ``` $ strace -f build/src/kcov coverage_dir tests/bash/handle-all-output.sh [ ...snip ... ] [pid 30363] <... read resumed> "550\n", 4096) = 4 [pid 30363] write(1, "550\n", 4550 ) = 4 [pid 30363] read(5, <unfinished ...> [pid 30366] <... write resumed> ) = 4 [pid 30366] write(782, "kcov@handle-all-output.sh@3@(( +"..., 38) = 38 [pid 30366] write(782, "kcov@handle-all-output.sh@3@(( i"..., 44) = 44 [pid 30366] write(782, "kcov@handle-all-output.sh@4@echo"..., 37 ``` At this point the kcov (pid 30363) and Bash (pid 30366) processes hang indefinitely. The new `bash_handle_all_output` test case reliably reproduces the failure and validates the fix from the previous commit. It required adding a timeout capability to `KcovTestCase.do`. I've also removed `@unittest.expectedFailure` from `bash_subshell`, as it isn't necessary and confirms that the desired behavior implemented by SimonKagstrom#193 remains intact.
SimonKagstrom
referenced
this pull request
Jul 22, 2017
bash-engine: Check stdout readable before getline
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.
Should fix #192