launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/more-robust-debdiff-timeout into lp:launchpad.

Commit message:
Use timeout(1) to limit debdiff rather than using alarm(3) ourselves.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/more-robust-debdiff-timeout/+merge/369535

The approach of calling alarm(3) and then execing the target command relies on the target command not changing the disposition of SIGALRM, and may have other problems (we've observed PackageDiffJobs occasionally hanging indefinitely, although we don't know exactly why that's happening).  coreutils has a perfectly good timeout(1) program that we can use instead, which runs the target command as a child process and in general seems likely to be more robust.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/more-robust-debdiff-timeout into lp:launchpad.

=== modified file 'lib/lp/soyuz/model/packagediff.py'
--- lib/lp/soyuz/model/packagediff.py	2015-12-02 10:57:23 +0000
+++ lib/lp/soyuz/model/packagediff.py	2019-07-01 16:27:41 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -13,7 +13,6 @@
 import os
 import resource
 import shutil
-import signal
 import subprocess
 import tempfile
 
@@ -47,13 +46,11 @@
     )
 
 
-def limit_deb_diff(timeout, max_size):
+def limit_deb_diff(max_size):
     """Pre-exec function to apply resource limits to debdiff.
 
-    :param timeout: Time limit in seconds.
     :param max_size: Maximum output file size in bytes.
     """
-    signal.alarm(timeout)
     _, hard_fsize = resource.getrlimit(resource.RLIMIT_FSIZE)
     if hard_fsize != resource.RLIM_INFINITY and hard_fsize < max_size:
         max_size = hard_fsize
@@ -83,7 +80,10 @@
                   if name.lower().endswith('.dsc')]
     [to_dsc] = [name for name in to_files
                 if name.lower().endswith('.dsc')]
-    args = ['debdiff', from_dsc, to_dsc]
+    args = [
+        'timeout', str(config.packagediff.debdiff_timeout),
+        'debdiff', from_dsc, to_dsc,
+        ]
     env = os.environ.copy()
     env['TMPDIR'] = tmp_dir
 
@@ -94,9 +94,7 @@
         process = subprocess.Popen(
             args, stdout=out_file, stderr=subprocess.PIPE,
             preexec_fn=partial(
-                limit_deb_diff,
-                config.packagediff.debdiff_timeout,
-                config.packagediff.debdiff_max_size),
+                limit_deb_diff, config.packagediff.debdiff_max_size),
             cwd=tmp_dir, env=env)
         stdout, stderr = process.communicate()
     finally:

=== modified file 'lib/lp/soyuz/tests/test_packagediff.py'
--- lib/lp/soyuz/tests/test_packagediff.py	2018-02-02 03:14:35 +0000
+++ lib/lp/soyuz/tests/test_packagediff.py	2019-07-01 16:27:41 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test source package diffs."""
@@ -173,6 +173,9 @@
         with open(mock_debdiff_path, "w") as mock_debdiff:
             print(dedent("""\
                 #! /bin/sh
+                # Make sure we don't rely on the child leaving its SIGALRM
+                # disposition undisturbed.
+                trap '' ALRM
                 (echo "$$"; echo "$TMPDIR") >%s
                 sleep 5
                 """ % marker_path), end="", file=mock_debdiff)