diff mbox

[kteam-tools] ktl/shell: always wait for thread to finish

Message ID 20170426134429.4998-1-cascardo@canonical.com
State New
Headers show

Commit Message

Thadeu Lima de Souza Cascardo April 26, 2017, 1:44 p.m. UTC
link-to-tracker was failing many times when getting the current branch,
because the output of the git command was empty. In fact, this popen
wrapper was not waiting for the command to finish, subject to race
conditions.

Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
---
 ktl/shell.py  | 4 ++++
 ktl3/shell.py | 4 ++++
 2 files changed, 8 insertions(+)

Comments

Colin Ian King April 26, 2017, 1:48 p.m. UTC | #1
On 26/04/17 14:44, Thadeu Lima de Souza Cascardo wrote:
> link-to-tracker was failing many times when getting the current branch,
> because the output of the git command was empty. In fact, this popen
> wrapper was not waiting for the command to finish, subject to race
> conditions.
> 
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
> ---
>  ktl/shell.py  | 4 ++++
>  ktl3/shell.py | 4 ++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/ktl/shell.py b/ktl/shell.py
> index ba5999eb..19d142cc 100644
> --- a/ktl/shell.py
> +++ b/ktl/shell.py
> @@ -88,6 +88,10 @@ def sh(cmd, timeout=None, ignore_result=False, quiet=False):
>          if t.is_alive():
>              p.terminate()
>              raise ShellTimeoutError(cmd, timeout)
> +    else:
> +        # If a timeout has not been specified, we still need to wait for
> +        # the thread to finish, but just don't care how long we wait.
> +        t.join()
>  
>      while p.poll() is None:
>          # read line without blocking
> diff --git a/ktl3/shell.py b/ktl3/shell.py
> index 7d70e5c3..72f21052 100644
> --- a/ktl3/shell.py
> +++ b/ktl3/shell.py
> @@ -97,6 +97,10 @@ def sh(cmd, timeout=None, ignore_result=False, quiet=False):
>              p.terminate()
>              cleave("sh")
>              raise ShellTimeoutError(cmd, timeout)
> +    else:
> +        # If a timeout has not been specified, we still need to wait for
> +        # the thread to finish, but just don't care how long we wait.
> +        t.join()
>  
>      while p.poll() is None:
>          # read line without blocking
> 

Yep, makes sense and avoiding races is paramount.

Acked-by: Colin Ian King <colin.king@canonical.com>
Stefan Bader April 26, 2017, 2:29 p.m. UTC | #2

Stefan Bader April 27, 2017, 9:26 a.m. UTC | #3

diff mbox

Patch

diff --git a/ktl/shell.py b/ktl/shell.py
index ba5999eb..19d142cc 100644
--- a/ktl/shell.py
+++ b/ktl/shell.py
@@ -88,6 +88,10 @@  def sh(cmd, timeout=None, ignore_result=False, quiet=False):
         if t.is_alive():
             p.terminate()
             raise ShellTimeoutError(cmd, timeout)
+    else:
+        # If a timeout has not been specified, we still need to wait for
+        # the thread to finish, but just don't care how long we wait.
+        t.join()
 
     while p.poll() is None:
         # read line without blocking
diff --git a/ktl3/shell.py b/ktl3/shell.py
index 7d70e5c3..72f21052 100644
--- a/ktl3/shell.py
+++ b/ktl3/shell.py
@@ -97,6 +97,10 @@  def sh(cmd, timeout=None, ignore_result=False, quiet=False):
             p.terminate()
             cleave("sh")
             raise ShellTimeoutError(cmd, timeout)
+    else:
+        # If a timeout has not been specified, we still need to wait for
+        # the thread to finish, but just don't care how long we wait.
+        t.join()
 
     while p.poll() is None:
         # read line without blocking