Add download and command timeouts and user feedback (#17)

* Add download and command timeouts and user feedback

* Move to variables as suggested by @amureki

lintipy.py

@@ -3,11 +3,11 @@
 import logging
 import os
 import resource
+import subprocess  # nosec
 import tarfile
 import tempfile
 import time
 from io import BytesIO
-from subprocess import Popen, PIPE, STDOUT  # nosec
 from urllib.parse import urlencode
 
 import boto3
@@ -43,9 +43,11 @@ class Handler:
         'opened', 'edited', 'reopened', 'synchronize',
     ]
 
+    FAQ_URL = 'https://lambdalint.github.io/#faq'
+
     def __init__(self, label: str, cmd: str, *cmd_args: str,
                  integration_id: str = None, bucket: str = None,
-                 region: str = None, pem: str = None):
+                 region: str = None, pem: str = None, cmd_timeout=200, download_timeout=30):
         self.label = label
         self.cmd = cmd
         self.cmd_args = cmd_args
@@ -57,6 +59,8 @@ def __init__(self, label: str, cmd: str, *cmd_args: str,
         self.integration_id = integration_id or os.environ.get('INTEGRATION_ID')
         self.bucket = bucket or os.environ.get('BUCKET', 'lambdalint')
         self.region = region or os.environ.get('REGION', 'eu-west-1')
+        self.cmd_timeout = cmd_timeout
+        self.download_timeout = download_timeout
 
         pem = pem or os.environ.get('PEM', '')
         self.pem = '\n'.join(pem.split('\\n'))
@@ -217,44 +221,55 @@ def run_process(self, code_path):
 
         """
         logger.info('Running: %s %s', self.cmd, ' '.join(self.cmd_args))
-        process = Popen(
-            ('python', '-m', self.cmd) + self.cmd_args,
-            stdout=PIPE, stderr=STDOUT,
-            cwd=code_path, env=self.get_env(),
-        )
-        process.wait()
-        info = resource.getrusage(resource.RUSAGE_CHILDREN)
-        log = process.stdout.read()
-        logger.debug(log)
-        logger.debug('exit %s', process.returncode)
-        logger.info('Saving log to S3')
-        key = os.path.join(self.label, self.full_name, "%s.log" % self.sha)
-        self.s3.put_object(
-            ACL='public-read',
-            Bucket=self.bucket,
-            Key=key,
-            Body=log,
-            ContentType='text/plain'
-        )
-        logger.info(
-            'linter exited with status code %s in %ss' % (process.returncode, info.ru_utime)
-        )
-        return (
-            info.ru_utime,
-            process.returncode,
-            "https://{0}.s3.amazonaws.com/{1}".format(self.bucket, key),
-        )
+        try:
+            process = subprocess.run(
+                ('python', '-m', self.cmd) + self.cmd_args,
+                stdout=subprocess.PIPE, stderr=subprocess.STDOUT,
+                cwd=code_path, env=self.get_env(),
+                timeout=self.cmd_timeout,
+            )
+        except subprocess.TimeoutExpired:
+            self.set_status(ERROR, 'Command timed out after %ss' % self.cmd_timeout, self.FAQ_URL)
+            raise
+        else:
+            info = resource.getrusage(resource.RUSAGE_CHILDREN)
+            log = process.stdout
+            logger.debug(log)
+            logger.debug('exit %s', process.returncode)
+            logger.info('Saving log to S3')
+            key = os.path.join(self.label, self.full_name, "%s.log" % self.sha)
+            self.s3.put_object(
+                ACL='public-read',
+                Bucket=self.bucket,
+                Key=key,
+                Body=log,
+                ContentType='text/plain'
+            )
+            logger.info(
+                'linter exited with status code %s in %ss' % (process.returncode, info.ru_utime)
+            )
+            return (
+                info.ru_utime,
+                process.returncode,
+                "https://{0}.s3.amazonaws.com/{1}".format(self.bucket, key),
+            )
 
     def download_code(self):
         """Download code to local filesystem storage."""
         logger.info('Downloading: %s', self.archive_url)
-        response = self.session.get(self.archive_url)
-        response.raise_for_status()
-        with BytesIO() as bs:
-            bs.write(response.content)
-            bs.seek(0)
-            path = tempfile.mkdtemp()
-            with tarfile.open(fileobj=bs, mode='r:gz') as fs:
-                fs.extractall(path)
-            folder = os.listdir(path)[0]
-            return os.path.join(path, folder)
+        try:
+            response = self.session.get(self.archive_url, timeout=self.download_timeout)
+        except requests.Timeout:
+            self.set_status(ERROR, 'Downloading code timed out after %ss' % self.download_timeout,
+                            self.FAQ_URL)
+            raise
+        else:
+            response.raise_for_status()
+            with BytesIO() as bs:
+                bs.write(response.content)
+                bs.seek(0)
+                path = tempfile.mkdtemp()
+                with tarfile.open(fileobj=bs, mode='r:gz') as fs:
+                    fs.extractall(path)
+                folder = os.listdir(path)[0]
+                return os.path.join(path, folder)

tests/conftest.py

@@ -27,7 +27,7 @@ def pull_request_event():
 
 @pytest.fixture(params=[push_event, pull_request_event])
 def handler(request, sns):
-    hnd = Handler('some linter', 'echo', '1', '2', '3')
+    hnd = Handler('zen of python', 'this', '1', '2', '3')
     subject, message = request.param()
     sns['Records'][0]['Sns']['Subject'] = subject
     sns['Records'][0]['Sns']['Message'] = message

tests/test_handler.py

@@ -1,19 +1,20 @@
 import logging
+import subprocess
 from unittest.mock import Mock
 
 import httpretty
 import pytest
 from botocore.vendored import requests
 
-from lintipy import Handler
+from lintipy import Handler, ERROR
 
 
 class TestHandler:
 
     def test_init(self):
-        hnd = Handler('some linter', 'echo', '1', '2', '3')
-        assert hnd.label == 'some linter'
-        assert hnd.cmd == 'echo'
+        hnd = Handler('zen of python', 'this', '1', '2', '3')
+        assert hnd.label == 'zen of python'
+        assert hnd.cmd == 'this'
         assert hnd.cmd_args == ('1', '2', '3')
 
     def test_hook(self, handler):
@@ -61,14 +62,54 @@ def test_call(self, handler, caplog):
         handler._session = requests.Session()
         handler._s3 = Mock()
         handler.download_code = lambda: '.'
+        with caplog.at_level(logging.INFO, logger='lintipy'):
+            handler(handler.event, {})
+        assert "linter exited with status code 0 in " in caplog.text
+
+        handler.cmd = 'doesnotexit'
         with caplog.at_level(logging.INFO, logger='lintipy'):
             handler(handler.event, {})
         assert "linter exited with status code 1 in " in caplog.text
 
+    @httpretty.activate
+    def test_timeout(self, handler, caplog):
+        httpretty.register_uri(
+            httpretty.POST, handler.statuses_url,
+            data='',
+            status=201,
+            content_type='application/json',
+        )
+        handler.cmd = 'tests.timeout'
+        handler.cmd_timeout = 1
+        handler._session = requests.Session()
+        handler._s3 = Mock()
+        handler.download_code = lambda: '.'
+        with pytest.raises(subprocess.TimeoutExpired) as e:
+            handler(handler.event, {})
+        assert "timed out after 1 seconds" in str(e)
+
     def test_download_code(self, handler):
         handler._session = requests.Session()
         assert handler.download_code()
 
+    def test_download_code_timeout(self, handler):
+        def _timeout(*args, **kwargs):
+            raise requests.Timeout('connection time out')
+
+        data = {}
+
+        def set_status(state, msg, target_url):
+            data['state'] = state
+            data['msg'] = msg
+        handler._session = requests.Session()
+        handler._session.get = _timeout
+        handler.download_timeout = float('1e-10')
+        handler.set_status = set_status
+        with pytest.raises(requests.Timeout):
+            handler.download_code()
+        assert data['state'] == ERROR
+        assert data['msg'] == 'Downloading code timed out after 1e-10s'
+
     @httpretty.activate
     def test_set_status(self, handler, caplog):
         httpretty.register_uri(
@@ -101,6 +142,6 @@ def test_set_status(self, handler, caplog):
     def test_get_log_url(self, handler):
         assert handler.get_log_url() == (
             'https://lambdalint.github.io/gh/?'
-            'app=some+linter&repo=baxterthehacker%2Fpublic-repo'
+            'app=zen+of+python&repo=baxterthehacker%2Fpublic-repo'
             '&ref=0d1a26e67d8f5eaf1f6ba5c57fc3c7d91ac0fd1c'
         )

tests/timeout.py

@@ -0,0 +1,3 @@
+import time
+
+time.sleep(2)