commands/globals: implement ExternalCommand.apply as coroutine

Rather than returning a deferred in some cases, this makes the function
a coroutine, in some cases it calls regular subprocess, in other cases
it uses asyncio subprocess.

2 files changed, 64 insertions, 39 deletions

diff --git a/alot/commands/globals.py b/alot/commands/globals.py
index 13eb04fd..8d8932bd 100644
--- a/alot/commands/globals.py
+++ b/alot/commands/globals.py
@@ -11,9 +11,9 @@ import os
 import subprocess
 from io import BytesIO
 import asyncio
+import shlex
 
 import urwid
-from twisted.internet import threads
 
 from . import Command, registerCommand
 from . import CommandCanceled
@@ -239,7 +239,7 @@ class ExternalCommand(Command):
         self.on_success = on_success
         Command.__init__(self, **kwargs)
 
-    def apply(self, ui):
+    async def apply(self, ui):
         logging.debug('cmdlist: %s', self.cmdlist)
         callerbuffer = ui.current_buffer
 
@@ -254,42 +254,59 @@ class ExternalCommand(Command):
             else:
                 stdin = self.stdin
 
-        def afterwards(data):
-            if data == 'success':
-                if self.on_success is not None:
-                    self.on_success()
-            else:
-                ui.notify(data, priority='error')
-            if self.refocus and callerbuffer in ui.buffers:
-                logging.info('refocussing')
-                ui.buffer_focus(callerbuffer)
-
         logging.info('calling external command: %s', self.cmdlist)
 
-        def thread_code(*_):
+        ret = ''
+        # TODO: these can probably be refactored in terms of helper.call_cmd
+        # and helper.call_cmd_async
+        if self.in_thread:
             try:
-                proc = subprocess.Popen(
-                    self.cmdlist, shell=self.shell,
-                    stdin=subprocess.PIPE if stdin else None,
-                    stderr=subprocess.PIPE)
+                if self.shell:
+                    _cmd = asyncio.create_subprocess_shell
+                    # The shell function wants a single string or bytestring,
+                    # we could just join it, but lets be extra safe and use
+                    # shlex.quote to avoid suprises.
+                    cmdlist = [shlex.quote(' '.join(self.cmdlist))]
+                else:
+                    _cmd = asyncio.create_subprocess_exec
+                    cmdlist = self.cmdlist
+                proc = await _cmd(
+                    *cmdlist,
+                    stdout=subprocess.PIPE,
+                    stderr=subprocess.PIPE,
+                    stdin=subprocess.PIPE if stdin else None)
             except OSError as e:
-                return str(e)
-
-            _, err = proc.communicate(stdin.read() if stdin else None)
+                ret = str(e)
+            else:
+                _, err = await proc.communicate(stdin.read() if stdin else None)
             if proc.returncode == 0:
-                return 'success'
-            if err:
-                return err.decode(urwid.util.detected_encoding)
-            return ''
-
-        if self.in_thread:
-            d = threads.deferToThread(thread_code)
-            d.addCallback(afterwards)
+                ret = 'success'
+            elif err:
+                ret = err.decode(urwid.util.detected_encoding)
         else:
             with ui.paused():
-                ret = thread_code()
-
-            afterwards(ret)
+                try:
+                    proc = subprocess.Popen(
+                        self.cmdlist, shell=self.shell,
+                        stdin=subprocess.PIPE if stdin else None,
+                        stderr=subprocess.PIPE)
+                except OSError as e:
+                    ret = str(e)
+                else:
+                    _, err = proc.communicate(stdin.read() if stdin else None)
+                if proc.returncode == 0:
+                    ret = 'success'
+                elif err:
+                    ret = err.decode(urwid.util.detected_encoding)
+
+        if ret == 'success':
+            if self.on_success is not None:
+                self.on_success()
+        else:
+            ui.notify(ret, priority='error')
+        if self.refocus and callerbuffer in ui.buffers:
+            logging.info('refocussing')
+            ui.buffer_focus(callerbuffer)
 
 
 class EditCommand(ExternalCommand):
diff --git a/tests/commands/global_test.py b/tests/commands/global_test.py
index b0e52913..88762799 100644
--- a/tests/commands/global_test.py
+++ b/tests/commands/global_test.py
@@ -176,47 +176,54 @@ class TestComposeCommand(unittest.TestCase):
 
 class TestExternalCommand(unittest.TestCase):
 
+    @inlineCallbacks
     def test_no_spawn_no_stdin_success(self):
         ui = utilities.make_ui()
         cmd = g_commands.ExternalCommand(u'true', refocus=False)
-        cmd.apply(ui)
+        yield ensureDeferred(cmd.apply(ui))
         ui.notify.assert_not_called()
 
+    @inlineCallbacks
     def test_no_spawn_stdin_success(self):
         ui = utilities.make_ui()
         cmd = g_commands.ExternalCommand(u"awk '{ exit $0 }'", stdin=u'0',
                                          refocus=False)
-        cmd.apply(ui)
+        yield ensureDeferred(cmd.apply(ui))
         ui.notify.assert_not_called()
 
+    @inlineCallbacks
     def test_no_spawn_no_stdin_attached(self):
         ui = utilities.make_ui()
         cmd = g_commands.ExternalCommand(u'test -t 0', refocus=False)
-        cmd.apply(ui)
+        yield ensureDeferred(cmd.apply(ui))
         ui.notify.assert_not_called()
 
+    @inlineCallbacks
     def test_no_spawn_stdin_attached(self):
         ui = utilities.make_ui()
         cmd = g_commands.ExternalCommand(
             u"test -t 0", stdin=u'0', refocus=False)
-        cmd.apply(ui)
+        yield ensureDeferred(cmd.apply(ui))
         ui.notify.assert_called_once_with('', priority='error')
 
+    @inlineCallbacks
     def test_no_spawn_failure(self):
         ui = utilities.make_ui()
         cmd = g_commands.ExternalCommand(u'false', refocus=False)
-        cmd.apply(ui)
+        yield ensureDeferred(cmd.apply(ui))
         ui.notify.assert_called_once_with('', priority='error')
 
+    @inlineCallbacks
     @mock.patch(
         'alot.commands.globals.settings.get', mock.Mock(return_value=''))
     @mock.patch.dict(os.environ, {'DISPLAY': ':0'})
     def test_spawn_no_stdin_success(self):
         ui = utilities.make_ui()
         cmd = g_commands.ExternalCommand(u'true', refocus=False, spawn=True)
-        cmd.apply(ui)
+        yield ensureDeferred(cmd.apply(ui))
         ui.notify.assert_not_called()
 
+    @inlineCallbacks
     @mock.patch(
         'alot.commands.globals.settings.get', mock.Mock(return_value=''))
     @mock.patch.dict(os.environ, {'DISPLAY': ':0'})
@@ -225,14 +232,15 @@ class TestExternalCommand(unittest.TestCase):
         cmd = g_commands.ExternalCommand(
             u"awk '{ exit $0 }'",
             stdin=u'0', refocus=False, spawn=True)
-        cmd.apply(ui)
+        yield ensureDeferred(cmd.apply(ui))
         ui.notify.assert_not_called()
 
+    @inlineCallbacks
     @mock.patch(
         'alot.commands.globals.settings.get', mock.Mock(return_value=''))
     @mock.patch.dict(os.environ, {'DISPLAY': ':0'})
     def test_spawn_failure(self):
         ui = utilities.make_ui()
         cmd = g_commands.ExternalCommand(u'false', refocus=False, spawn=True)
-        cmd.apply(ui)
+        yield ensureDeferred(cmd.apply(ui))
         ui.notify.assert_called_once_with('', priority='error')