Updated Branches: refs/heads/master 38eafa088 -> 2218261eb
BIGTOP-953. Revert BIGTOP-835 and BIGTOP-950. Revert "BIGTOP-950. race condition for output consumption in Shell code" "BIGTOP-835. The shell exec method must have variants which have timeout and can run in background" This reverts commits bc49df0bf3ed71dbe7a8ded7857b6087d4df8fd0 b5f900efc17c616dc34704cd520285adcd814e9e Project: http://git-wip-us.apache.org/repos/asf/bigtop/repo Commit: http://git-wip-us.apache.org/repos/asf/bigtop/commit/10fb276a Tree: http://git-wip-us.apache.org/repos/asf/bigtop/tree/10fb276a Diff: http://git-wip-us.apache.org/repos/asf/bigtop/diff/10fb276a Branch: refs/heads/master Commit: 10fb276aaab2e3a2fbae6cff241ee20c9eff684c Parents: 3f03f62 Author: Konstantin Boudnik <[email protected]> Authored: Wed May 1 13:53:48 2013 -0700 Committer: Konstantin Boudnik <[email protected]> Committed: Wed May 1 13:53:48 2013 -0700 ---------------------------------------------------------------------- .../org/apache/bigtop/itest/shell/Shell.groovy | 99 ++------------- .../org/apache/bigtop/itest/shell/ShellTest.groovy | 46 ------- 2 files changed, 9 insertions(+), 136 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/bigtop/blob/10fb276a/bigtop-test-framework/src/main/groovy/org/apache/bigtop/itest/shell/Shell.groovy ---------------------------------------------------------------------- diff --git a/bigtop-test-framework/src/main/groovy/org/apache/bigtop/itest/shell/Shell.groovy b/bigtop-test-framework/src/main/groovy/org/apache/bigtop/itest/shell/Shell.groovy index fbd85e3..ae3da68 100644 --- a/bigtop-test-framework/src/main/groovy/org/apache/bigtop/itest/shell/Shell.groovy +++ b/bigtop-test-framework/src/main/groovy/org/apache/bigtop/itest/shell/Shell.groovy @@ -62,13 +62,10 @@ class Shell { * stdout as getOut() and stderr as getErr(). The script itself can be accessed * as getScript() * WARNING: it isn't thread safe - * @param timeout timeout in milliseconds to wait before killing the script. - * If timeout < 0, then this method will wait until the script completes - * and will not be killed. * @param args shell script split into multiple Strings * @return Shell object for chaining */ - Shell execWithTimeout(int timeout, Object... args) { + Shell exec(Object... args) { def proc = user ? "sudo -u $user PATH=${System.getenv('PATH')} $shell".execute() : "$shell".execute() script = args.join("\n") @@ -81,31 +78,20 @@ class Shell { writer.println(script) writer.close() } - ByteArrayOutputStream outStream = new ByteArrayOutputStream(4096) - ByteArrayOutputStream errStream = new ByteArrayOutputStream(4096) - if (timeout >= 0) { - // WARNING: there's a potential race condition bellow - // essentially what we really need here is - // proc.waitForOrKillProcessOutput(outStream, errStream) - proc.consumeProcessOutput(outStream, errStream) - proc.waitForOrKill(timeout) - } else { - proc.waitForProcessOutput(outStream, errStream) - } + ByteArrayOutputStream baosErr = new ByteArrayOutputStream(4096); + proc.consumeProcessErrorStream(baosErr); + out = proc.in.readLines() // Possibly a bug in String.split as it generates a 1-element array on an // empty String - if (outStream.size() != 0) { - out = outStream.toString().split('\n') - } else { - out = Collections.EMPTY_LIST - } - if (errStream.size() != 0) { - err = errStream.toString().split('\n') + if (baosErr.size() != 0) { + err = baosErr.toString().split('\n'); } else { - err = Collections.EMPTY_LIST + err = new ArrayList<String>(); } + + proc.waitFor() ret = proc.exitValue() if (LOG.isTraceEnabled()) { @@ -119,74 +105,7 @@ class Shell { LOG.trace("\n<stderr>\n${err.join('\n')}\n</stderr>"); } } - return this - } - - /** - * Execute shell script consisting of as many Strings as we have arguments, - * possibly under an explicit username (requires sudoers privileges). - * NOTE: individual strings are concatenated into a single script as though - * they were delimited with new line character. All quoting rules are exactly - * what one would expect in standalone shell script. - * - * After executing the script its return code can be accessed as getRet(), - * stdout as getOut() and stderr as getErr(). The script itself can be accessed - * as getScript() - * WARNING: it isn't thread safe - * @param timeout timeout in milliseconds to wait before killing the script - * . If timeout < 0, then this method will wait until the script completes - * and will not be killed. - * @param args shell script split into multiple Strings - * @return Shell object for chaining - */ - Shell exec(Object... args) { - return execWithTimeout(-1, args) - } - - /** - * Executes a shell script consisting of as many strings as we have args, - * under an explicit user name. This method does the same job as - * {@linkplain #exec(java.lang.Object[])}, but will return immediately, - * with the process continuing execution in the background. If this method - * is called, the output stream and error stream of this script will be - * available in the {@linkplain #out} and {@linkplain #err} lists. - * WARNING: it isn't thread safe - * <strong>CAUTION:</strong> - * If this shell object is used to run other script while a script is - * being executed in the background, then the output stream and error - * stream of the script executed later will be what is available, - * and the output and error streams of this script may be lost. - * @param args - * @return Shell object for chaining - */ - Shell fork(Object... args) { - forkWithTimeout(-1, args) - return this - } - /** - * Executes a shell script consisting of as many strings as we have args, - * under an explicit user name. This method does the same job as - * {@linkplain #execWithTimeout(int, java.lang.Object[])}, but will return immediately, - * with the process continuing execution in the background for timeout - * milliseconds (or until the script completes , whichever is earlier). If - * this method - * is called, the output stream and error stream of this script will be - * available in the {@linkplain #out} and {@linkplain #err} lists. - * WARNING: it isn't thread safe - * <strong>CAUTION:</strong> - * If this shell object is used to run other script while a script is - * being executed in the background, then the output stream and error - * stream of the script executed later will be what is available, - * and the output and error streams of this script may be lost. - * @param timeout The timoeut in milliseconds before the script is killed - * @param args - * @return Shell object for chaining - */ - Shell forkWithTimeout(int timeout, Object... args) { - Thread.start { - execWithTimeout(timeout, args) - } return this } } http://git-wip-us.apache.org/repos/asf/bigtop/blob/10fb276a/bigtop-test-framework/src/test/groovy/org/apache/bigtop/itest/shell/ShellTest.groovy ---------------------------------------------------------------------- diff --git a/bigtop-test-framework/src/test/groovy/org/apache/bigtop/itest/shell/ShellTest.groovy b/bigtop-test-framework/src/test/groovy/org/apache/bigtop/itest/shell/ShellTest.groovy index 63ca7ee..1571e10 100644 --- a/bigtop-test-framework/src/test/groovy/org/apache/bigtop/itest/shell/ShellTest.groovy +++ b/bigtop-test-framework/src/test/groovy/org/apache/bigtop/itest/shell/ShellTest.groovy @@ -23,7 +23,6 @@ import org.junit.Test import static org.junit.Assert.assertEquals import static org.junit.Assert.assertFalse -import static org.junit.Assert.assertTrue class ShellTest { @Test @@ -49,49 +48,4 @@ class ShellTest { assertEquals("got extra stdout ${sh.out}", 0, sh.out.size()) assertEquals("got extra stderr ${sh.err}", 0, sh.err.size()) } - - @Test - void testRegularShellWithTimeout() { - Shell sh = new Shell("/bin/bash -s") - def preTime = System.currentTimeMillis() - sh.execWithTimeout(500, 'A=a ; sleep 30 ; r() { return $1; } ; echo $A ; ' + - 'r `id -u`') - assertTrue(System.currentTimeMillis() - preTime < 30000) - } - - @Test - void testBackgroundExecWithTimeoutFailure() { - Shell sh = new Shell("/bin/bash -s") - def preTime = System.currentTimeMillis() - sh.forkWithTimeout(500, 'A=a ; sleep 30 ; r() { return $1; } ' + - '; echo $A ; r `id -u`') - //Wait for script to get killed - Thread.sleep(750) - assertTrue("got wrong stdout ${sh.out}", sh.out.isEmpty()) - assertTrue("got wrong srderr ${sh.out}", sh.err.isEmpty()) - } - - @Test - void testForkWithTimeoutSuccess() { - Shell sh = new Shell("/bin/bash -s") - def preTime = System.currentTimeMillis() - sh.forkWithTimeout(30000, - 'A=a ; r() { return $1; } ; echo $A ; r `id -u`') - //Wait for the script to complete - Thread.sleep(500) - assertFalse("${sh.script} exited with a non-zero status", sh.ret == 0) - assertEquals("got wrong stdout ${sh.out}", "a", sh.out[0]) - assertEquals("got extra stderr ${sh.err}", 0, sh.err.size()) - } - - @Test - void testForkWithoutTimeoutSuccess() { - Shell sh = new Shell("/bin/bash -s") - sh.fork('A=a ; r() { return $1; } ; echo $A ; r `id -u`') - //Wait for the script to complete - Thread.sleep(550) - assertFalse("${sh.script} exited with a non-zero status", sh.ret == 0) - assertEquals("got wrong stdout ${sh.out}", "a", sh.out[0]) - assertEquals("got extra stderr ${sh.err}", 0, sh.err.size()) - } }
