apply util.Popen everywhere

It will default to close_fds=True.
On Windows, it will log an explanation message on incompatible use of close_fds+redirection.

share/gpodder/examples/hello_world.py

@@ -52,7 +52,7 @@ class gPodderExtension:
         self.gpodder.notification("Hello Extension", "Message", widget=self.gpodder.main_window)
 
 
-# Concurrency Warning
+# Concurrency Warning: use gpodder.util.Popen() instead of subprocess.Popen()
 #
 # When using subprocess.Popen() to spawn a long-lived external command,
 # such as ffmpeg, be sure to include the "close_fds=True" argument.
@@ -65,9 +65,10 @@ class gPodderExtension:
 # Otherwise that process will inherit ALL file descriptors gPodder
 # happens to have open at the moment (like other active downloads).
 # Those files will remain 'in-use' until that process exits, a race
-# condition which prevents gPodder from renaming or deleting them.
+# condition which prevents gPodder from renaming or deleting them on Windows.
 #
 # Caveat: On Windows, you cannot set close_fds to true and also
 # redirect the standard handles (stdin, stdout or stderr). To collect
 # output/errors from long-lived external commands, it may be necessary
 # to create a (temp) log file and read it afterward.
+#

share/gpodder/extensions/audio_converter.py

@@ -100,9 +100,14 @@ class gPodderExtension:
             [param % {'old_file': old_filename, 'new_file': new_filename}
                 for param in cmd_param]
 
-        ffmpeg = subprocess.Popen(cmd, stdout=subprocess.PIPE,
-                stderr=subprocess.PIPE)
-        stdout, stderr = ffmpeg.communicate()
+        if gpodder.ui.win32:
+            ffmpeg = util.Popen(cmd)
+            ffmpeg.wait()
+            stdout, stderr = ("<unavailable>",) * 2
+        else:
+            ffmpeg = util.Popen(cmd, stdout=subprocess.PIPE,
+                    stderr=subprocess.PIPE)
+            stdout, stderr = ffmpeg.communicate()
 
         if ffmpeg.returncode == 0:
             util.rename_episode_file(episode, new_filename)

share/gpodder/extensions/command_on_download.py

@@ -8,6 +8,8 @@ import logging
 import os
 import subprocess
 
+from gpodder import util
+
 logger = logging.getLogger(__name__)
 
 __title__ = 'Run a Command on Download'
@@ -64,7 +66,7 @@ class gPodderExtension:
         env = os.environ.copy()
         env.update(info)
 
-        proc = subprocess.Popen(command, shell=True, env=env, close_fds=True)
+        proc = util.Popen(command, shell=True, env=env, close_fds=True)
         proc.wait()
         if proc.returncode == 0:
             logger.info("%s succeeded", command)

share/gpodder/extensions/concatenate_videos.py

@@ -70,8 +70,9 @@ class gPodderExtension:
                 }, False, self.gpodder.get_dialog_parent())
 
         def convert():
-            ffmpeg = subprocess.Popen(['ffmpeg', '-f', 'concat', '-nostdin', '-y',
-                '-i', list_filename, '-c', 'copy', out_filename], close_fds=True)
+            ffmpeg = util.Popen(['ffmpeg', '-f', 'concat', '-nostdin', '-y',
+                                 '-i', list_filename, '-c', 'copy', out_filename],
+                                close_fds=True)
             result = ffmpeg.wait()
             util.delete_file(list_filename)
             util.idle_add(lambda: indicator.on_finished())

share/gpodder/extensions/enqueue_in_mediaplayer.py

@@ -58,8 +58,7 @@ class FreeDesktopPlayer(Player):
         return util.find_command(self.command[0]) is not None
 
     def open_files(self, filenames):
-        subprocess.Popen(self.command + filenames,
-                         stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+        util.Popen(self.command + filenames)
 
 
 class Win32Player(Player):
@@ -78,7 +77,7 @@ class Win32Player(Player):
 
     def open_files(self, filenames):
         for cmd in util.format_desktop_command(self.command, filenames):
-            subprocess.Popen(cmd, close_fds=True)
+            util.Popen(cmd, close_fds=True)
 
 
 class MPRISResumer(FreeDesktopPlayer):

share/gpodder/extensions/normalize_audio.py

@@ -91,9 +91,14 @@ class gPodderExtension:
 
         cmd = [CONVERT_COMMANDS.get(extension, 'normalize-audio'), filename]
 
-        p = subprocess.Popen(cmd, stdout=subprocess.PIPE,
-                stderr=subprocess.PIPE)
-        stdout, stderr = p.communicate()
+        if gpodder.ui.win32:
+            p = util.Popen(cmd)
+            p.wait()
+            stdout, stderr = ("<unavailable>",) * 2
+        else:
+            p = util.Popen(cmd, stdout=subprocess.PIPE,
+                    stderr=subprocess.PIPE)
+            stdout, stderr = p.communicate()
 
         if p.returncode == 0:
             logger.info('normalize-audio processing successful.')

share/gpodder/extensions/rockbox_convert2mp4.py

@@ -132,9 +132,14 @@ class gPodderExtension:
             'options': self.container.config.ffmpeg_options
         }
 
-        process = subprocess.Popen(shlex.split(convert_command),
-            stdout=subprocess.PIPE, stderr=subprocess.PIPE)
-        stdout, stderr = process.communicate()
+        if gpodder.ui.win32:
+            p = util.Popen(shlex.split(convert_command))
+            p.wait()
+            stdout, stderr = ("<unavailable>",) * 2
+        else:
+            process = util.Popen(shlex.split(convert_command),
+                                 stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+            stdout, stderr = process.communicate()
         if process.returncode != 0:
             logger.error(stderr)
             return None

share/gpodder/extensions/ubuntu_unity.py

@@ -4,6 +4,7 @@
 # Thomas Perl <thp@gpodder.org>; 2012-02-06
 
 import gpodder
+from gpodder import util
 
 _ = gpodder.gettext
 
@@ -40,8 +41,8 @@ if __name__ != '__main__':
         def on_load(self):
             logger.info('Starting Ubuntu Unity Integration.')
             os.environ['PYTHONPATH'] = os.pathsep.join(sys.path)
-            self.process = subprocess.Popen(['python', __file__],
-                    stdin=subprocess.PIPE)
+            self.process = util.Popen(['python', __file__],
+                                             stdin=subprocess.PIPE)
 
         def on_unload(self):
             logger.info('Killing process...')

share/gpodder/extensions/video_converter.py

@@ -99,9 +99,15 @@ class gPodderExtension:
         cmd = [self.command] + \
             [param % {'old_file': old_filename, 'new_file': new_filename}
                 for param in self.command_param]
-        ffmpeg = subprocess.Popen(cmd, stdout=subprocess.PIPE,
-                stderr=subprocess.PIPE)
-        stdout, stderr = ffmpeg.communicate()
+
+        if gpodder.ui.win32:
+            ffmpeg = util.Popen(cmd)
+            ffmpeg.wait()
+            stdout, stderr = ("<unavailable>",) * 2
+        else:
+            ffmpeg = util.Popen(cmd, stdout=subprocess.PIPE,
+                    stderr=subprocess.PIPE)
+            stdout, stderr = ffmpeg.communicate()
 
         if ffmpeg.returncode == 0:
             util.rename_episode_file(episode, new_filename)

src/gpodder/gtkui/main.py

@@ -2068,7 +2068,7 @@ class gPodder(BuilderWidget, dbus.service.Object):
                         for command in util.format_desktop_command('panucci',
                                 [filename]):
                             logger.info('Executing: %s', repr(command))
-                            subprocess.Popen(command, close_fds=True)
+                            util.Popen(command, close_fds=True)
 
                     def on_error(err):
                         return error_handler(filename, err)
@@ -2094,7 +2094,7 @@ class gPodder(BuilderWidget, dbus.service.Object):
         for group in groups:
             for command in util.format_desktop_command(group, groups[group], resume_position):
                 logger.debug('Executing: %s', repr(command))
-                subprocess.Popen(command, close_fds=True)
+                util.Popen(command, close_fds=True)
 
         # Persist episode status changes to the database
         self.db.commit()

src/gpodder/util.py

@@ -1291,7 +1291,7 @@ def bluetooth_send_file(filename):
 
     if command_line is not None:
         command_line.append(filename)
-        return (subprocess.Popen(command_line, close_fds=True).wait() == 0)
+        return (Popen(command_line, close_fds=True).wait() == 0)
     else:
         logger.error('Cannot send file. Please install "bluetooth-sendto" or "gnome-obex-send".')
         return False
@@ -1420,9 +1420,9 @@ def gui_open(filename):
         if gpodder.ui.win32:
             os.startfile(filename)
         elif gpodder.ui.osx:
-            subprocess.Popen(['open', filename], close_fds=True)
+            Popen(['open', filename], close_fds=True)
         else:
-            subprocess.Popen(['xdg-open', filename], close_fds=True)
+            Popen(['xdg-open', filename], close_fds=True)
         return True
     except:
         logger.error('Cannot open file/folder: "%s"', filename, exc_info=True)
@@ -1804,7 +1804,7 @@ def linux_get_active_interfaces():
     empty list if the device is offline. The loopback
     interface is not included.
     """
-    process = subprocess.Popen(['ip', 'link'], stdout=subprocess.PIPE)
+    process = Popen(['ip', 'link'], close_fds=True, stdout=subprocess.PIPE)
     data, _ = process.communicate()
     for interface, _ in re.findall(r'\d+: ([^:]+):.*state (UP|UNKNOWN)', data.decode(locale.getpreferredencoding())):
         if interface != 'lo':
@@ -1818,7 +1818,7 @@ def osx_get_active_interfaces():
     empty list if the device is offline. The loopback
     interface is not included.
     """
-    process = subprocess.Popen(['ifconfig'], stdout=subprocess.PIPE)
+    process = Popen(['ifconfig'], close_fds=True, stdout=subprocess.PIPE)
     stdout, _ = process.communicate()
     for i in re.split('\n(?!\t)', stdout.decode('utf-8'), re.MULTILINE):
         b = re.match('(\\w+):.*status: (active|associated)$', i, re.MULTILINE | re.DOTALL)
@@ -1833,7 +1833,7 @@ def unix_get_active_interfaces():
     empty list if the device is offline. The loopback
     interface is not included.
     """
-    process = subprocess.Popen(['ifconfig'], stdout=subprocess.PIPE)
+    process = Popen(['ifconfig'], close_fds=True, stdout=subprocess.PIPE)
     stdout, _ = process.communicate()
     for i in re.split('\n(?!\t)', stdout.decode(locale.getpreferredencoding()), re.MULTILINE):
         b = re.match('(\\w+):.*status: (active|associated)$', i, re.MULTILINE | re.DOTALL)
@@ -1960,3 +1960,93 @@ def iri_to_url(url):
     url[2] = urllib.parse.quote(url[2], safe="/-._~!$&'()*+,;=:@")
     url = urllib.parse.urlunsplit(url)
     return url
+
+
+class Popen(subprocess.Popen):
+
+    """A Popen process that tries not to leak file descriptors.
+
+    This is a drop-in replacement for subprocess.Popen(), which takes the same
+    arguments.
+
+    'close_fds' will default to True, if omitted. This stops the process from
+    inheriting ALL of gPodder's file descriptors, which would keep them
+    'in-use'. That is of particular concern whenever the download queue is
+    active and interacting with the filesystem in the background.
+
+    On Windows however, redirection cannot coexist with 'close_fds=True'.
+    Specifying both will raise a ValueError. A message will appear in the log.
+
+    For communication with short-lived Windows commands, setting 'close_fds'
+    to False may be a tolerable risk. Otherwise as a last resort, sending
+    output to temp files to read afterward might work (probably involving
+    'shell=True').
+
+    See https://github.com/gpodder/gpodder/issues/420
+    """
+
+    def __init__(self, *args, **kwargs):
+        self.__logged_returncode = False
+
+        if 'close_fds' not in kwargs:
+            kwargs['close_fds'] = True
+
+        try:
+            super(Popen, self).__init__(*args, **kwargs)  #Python 2 syntax
+
+        except (ValueError) as e:
+            if gpodder.ui.win32 and kwargs['close_fds']:
+                if [(k, v) for (k, v) in kwargs.items() if k in ('stdin', 'stdout', 'stderr') and v]:
+                    logger = logging.getLogger(__name__)
+                    logger.error('util.Popen(close_fds=True) is incompatible with stream redirection on Windows.')
+                    logger.error('With close_fds=False, the process keeps all currently open files locked. It might be tolerable for short-lived commands. Or use temp files.')
+
+            raise e
+
+    @classmethod
+    def testPopen():
+        # Commands that will complain on stderr.
+        if gpodder.ui.win32:
+            cmd = ['findstr.exe', '/!']
+            cmd_pipe = ['findstr', 'hello']
+        else:
+            cmd = ['cat', '--helpp']
+            cmd_pipe = ['grep', 'hello']
+
+        logger.info('Test #1: Implicit close_fds=True, with no redirection')
+        logger.info('No race condition.')
+        logger.info('Streams left in the console.')
+        logger.info('Typical spawn and forget. Might as well wait().')
+        p = Popen(cmd)
+        out, err = p.communicate()
+        print("- - stderr - -\n{}\n- - -    - - -\n".format(err))
+
+        logger.info('Test #2: Explicit close_fds=False, with redirection.')
+        logger.info('This has a race condition, but communicate() always returns streams.')
+        p = Popen(cmd, close_fds=False, stderr=subprocess.PIPE, universal_newlines=True)
+        out, err = p.communicate()
+        print("- - stderr - -\n{}\n- - -    - - -\n".format(err))
+
+        try:
+            logger.info('Test #3: Implicit close_fds=True, with attempted redirection.')
+            logger.info('No race condition.')
+            logger.info('On Windows, this will raise ValueError.')
+            logger.info('Other platforms will have readable streams returned.')
+            p = Popen(cmd, stderr=subprocess.PIPE, universal_newlines=True)
+            out, err = p.communicate()
+            print("- - stderr - -\n{}\n- - -    - - -\n".format(err))
+
+        except (ValueError) as e:
+            print("- - Caught - -\n{}: {}\n- - -    - - -\n".format(e.__class__.__name__, e))
+
+        try:
+            logger.info('Test #4: Implicit close_fds=True, given input.')
+            p = Popen(cmd_pipe, stdin=subprocess.PIPE)
+            out, err = p.communicate(input=b'hello world')
+            print("NEVER REACHED ON WINDOWS")
+            print("- - stderr - -\n{}\n- - -    - - -\n".format(err))
+
+        except (ValueError) as e:
+            print("- - Caught - -\n{}: {}\n- - -    - - -\n".format(e.__class__.__name__, e))
+
+        logger.info('Log spam only occurs if returncode is non-zero or if explaining the Windows redirection error.')