Facts Timeout was not settable via ansible.cfg

The timeout for gathering facts needs to be settable from three places
(highest precedence to lowest):

* programmatically
* ansible.cfg (equivalent to the user specifying it explicitly when
  calling setup)
* from the default value

The code was changed in b4bd6c8 to
allow programmatically and the default value to work correctly but
setting via ansible.cfg/parameter was broken.

This change should fix setting via ansible.cfg and adds unittests for
all three cases

Fixes ansible#23753

lib/ansible/module_utils/facts.py

@@ -77,22 +77,24 @@
 # steps do not exceed a time limit
 
 GATHER_TIMEOUT=None
+DEFAULT_GATHER_TIMEOUT = 10
 
 class TimeoutError(Exception):
     pass
 
 def timeout(seconds=None, error_message="Timer expired"):
 
-    if seconds is None:
-        seconds = globals().get('GATHER_TIMEOUT') or 10
-
     def decorator(func):
         def _handle_timeout(signum, frame):
             raise TimeoutError(error_message)
 
         def wrapper(*args, **kwargs):
+            local_seconds = seconds  # Make local var as we modify this every time it's invoked
+            if local_seconds is None:
+                local_seconds = globals().get('GATHER_TIMEOUT') or DEFAULT_GATHER_TIMEOUT
+
             signal.signal(signal.SIGALRM, _handle_timeout)
-            signal.alarm(seconds)
+            signal.alarm(local_seconds)
             try:
                 result = func(*args, **kwargs)
             finally:
@@ -103,11 +105,11 @@ def wrapper(*args, **kwargs):
 
     # If we were called as @timeout, then the first parameter will be the
     # function we are to wrap instead of the number of seconds.  Detect this
-    # and correct it by setting seconds to our default value and return the
+    # and correct it by setting seconds to our sentinel value and return the
     # inner decorator function manually wrapped around the function
     if callable(seconds):
         func = seconds
-        seconds = 10
+        seconds = None
         return decorator(func)
 
     # If we were called as @timeout([...]) then python itself will take

test/units/module_utils/facts/test_timeout.py

@@ -0,0 +1,112 @@
+# -*- coding: utf-8 -*-
+# (c) 2017, Toshio Kuratomi <[email protected]>
+#
+# This file is part of Ansible
+#
+# Ansible is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# Ansible is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with Ansible.  If not, see <http://www.gnu.org/licenses/>.
+
+# Make coding more python3-ish
+from __future__ import (absolute_import, division)
+__metaclass__ = type
+
+import time
+
+import pytest
+
+from ansible.compat.tests import unittest
+from ansible.compat.tests.mock import patch, MagicMock
+
+from ansible.module_utils import facts
+
+
+@pytest.fixture
+def set_gather_timeout_higher():
+    default_timeout = facts.GATHER_TIMEOUT
+    facts.GATHER_TIMEOUT = facts.DEFAULT_GATHER_TIMEOUT + 5
+    yield
+    facts.GATHER_TIMEOUT = default_timeout
+
+
+@pytest.fixture
+def set_gather_timeout_lower():
+    default_timeout = facts.GATHER_TIMEOUT
+    facts.GATHER_TIMEOUT = 2
+    yield
+    facts.GATHER_TIMEOUT = default_timeout
+
+
+@facts.timeout
+def sleep_amount_implicit(amount):
+    # implicit refers to the lack of argument to the decorator
+    time.sleep(amount)
+    return 'Succeeded after {0} sec'.format(amount)
+
+
+@facts.timeout(facts.DEFAULT_GATHER_TIMEOUT + 5)
+def sleep_amount_explicit_higher(amount):
+    # explicit refers to the argument to the decorator
+    time.sleep(amount)
+    return 'Succeeded after {0} sec'.format(amount)
+
+
+@facts.timeout(2)
+def sleep_amount_explicit_lower(amount):
+    # explicit refers to the argument to the decorator
+    time.sleep(amount)
+    return 'Succeeded after {0} sec'.format(amount)
+
+
+def test_defaults_still_within_bounds():
+    # If the default changes outside of these bounds, some of the tests will
+    # no longer test the right thing.  Need to review and update the timeouts
+    # in the other tests if this fails
+    assert facts.DEFAULT_GATHER_TIMEOUT >= 4
+
+
+def test_implicit_file_default_succeeds():
+    # amount checked must be less than DEFAULT_GATHER_TIMEOUT
+    assert sleep_amount_implicit(1) == 'Succeeded after 1 sec'
+
+
+def test_implicit_file_default_timesout():
+    # sleep_time is greater than the default
+    sleep_time = facts.DEFAULT_GATHER_TIMEOUT + 1
+    with pytest.raises(facts.TimeoutError):
+        assert sleep_amount_implicit(sleep_time) == '(Not expected to succeed)'
+
+
+def test_implicit_file_overridden_succeeds(set_gather_timeout_higher):
+    # Set sleep_time greater than the default timeout and less than our new timeout
+    sleep_time = facts.DEFAULT_GATHER_TIMEOUT + 1
+    assert sleep_amount_implicit(sleep_time) == 'Succeeded after {0} sec'.format(sleep_time)
+
+
+def test_implicit_file_overridden_timesout(set_gather_timeout_lower):
+    # Set sleep_time greater than our new timeout but less than the default
+    sleep_time = 3
+    with pytest.raises(facts.TimeoutError):
+        assert sleep_amount_implicit(sleep_time) == '(Not expected to Succeed)'
+
+
+def test_explicit_succeeds():
+    # Set sleep_time greater than the default timeout and less than our new timeout
+    sleep_time = facts.DEFAULT_GATHER_TIMEOUT + 1
+    assert sleep_amount_explicit_higher(sleep_time) == 'Succeeded after {0} sec'.format(sleep_time)
+
+
+def test_explicit_timeout():
+    # Set sleep_time greater than our new timeout but less than the default
+    sleep_time = 3
+    with pytest.raises(facts.TimeoutError):
+        assert sleep_amount_explicit_lower(sleep_time) == '(Not expected to succeed)'