Merge pull request #2220 from letsencrypt/revert-2111-bugs/1719

Revert "Fix "global" max_attempt bug (#1719)"

acme/acme/client.py

@@ -1,5 +1,4 @@
 """ACME client API."""
-import collections
 import datetime
 import heapq
 import logging
@@ -335,9 +334,8 @@ class Client(object):  # pylint: disable=too-many-instance-attributes
         :param authzrs: `list` of `.AuthorizationResource`
         :param int mintime: Minimum time before next attempt, used if
             ``Retry-After`` is not present in the response.
-        :param int max_attempts: Maximum number of attempts (per
-            authorization) before `PollError` with non-empty ``waiting``
-            is raised.
+        :param int max_attempts: Maximum number of attempts before
+            `PollError` with non-empty ``waiting`` is raised.
 
         :returns: ``(cert, updated_authzrs)`` `tuple` where ``cert`` is
             the issued certificate (`.messages.CertificateResource`),
@@ -351,11 +349,6 @@ class Client(object):  # pylint: disable=too-many-instance-attributes
             was marked by the CA as invalid
 
         """
-        # pylint: disable=too-many-locals
-        assert max_attempts > 0
-        attempts = collections.defaultdict(int)
-        exhausted = set()
-
         # priority queue with datetime (based on Retry-After) as key,
         # and original Authorization Resource as value
         waiting = [(datetime.datetime.now(), authzr) for authzr in authzrs]
@@ -363,7 +356,8 @@ class Client(object):  # pylint: disable=too-many-instance-attributes
         # recently updated one
         updated = dict((authzr, authzr) for authzr in authzrs)
 
-        while waiting:
+        while waiting and max_attempts:
+            max_attempts -= 1
             # find the smallest Retry-After, and sleep if necessary
             when, authzr = heapq.heappop(waiting)
             now = datetime.datetime.now()
@@ -377,20 +371,16 @@ class Client(object):  # pylint: disable=too-many-instance-attributes
             updated_authzr, response = self.poll(updated[authzr])
             updated[authzr] = updated_authzr
 
-            attempts[authzr] += 1
             # pylint: disable=no-member
             if updated_authzr.body.status not in (
                     messages.STATUS_VALID, messages.STATUS_INVALID):
-                if attempts[authzr] < max_attempts:
-                    # push back to the priority queue, with updated retry_after
-                    heapq.heappush(waiting, (self.retry_after(
-                        response, default=mintime), authzr))
-                else:
-                    exhausted.add(authzr)
+                # push back to the priority queue, with updated retry_after
+                heapq.heappush(waiting, (self.retry_after(
+                    response, default=mintime), authzr))
 
-        if exhausted or any(authzr.body.status == messages.STATUS_INVALID
-                            for authzr in six.itervalues(updated)):
-            raise errors.PollError(exhausted, updated)
+        if not max_attempts or any(authzr.body.status == messages.STATUS_INVALID
+                                   for authzr in six.itervalues(updated)):
+            raise errors.PollError(waiting, updated)
 
         updated_authzrs = tuple(updated[authzr] for authzr in authzrs)
         return self.request_issuance(csr, updated_authzrs), updated_authzrs

acme/acme/client_test.py

@@ -319,10 +319,7 @@ class ClientTest(unittest.TestCase):
         )
 
         cert, updated_authzrs = self.client.poll_and_request_issuance(
-            csr, authzrs, mintime=mintime,
-            # make sure that max_attempts is per-authorization, rather
-            # than global
-            max_attempts=max(len(authzrs[0].retries), len(authzrs[1].retries)))
+            csr, authzrs, mintime=mintime)
         self.assertTrue(cert[0] is csr)
         self.assertTrue(cert[1] is updated_authzrs)
         self.assertEqual(updated_authzrs[0].uri, 'a...')

acme/acme/errors.py

@@ -56,25 +56,26 @@ class MissingNonce(NonceError):
 class PollError(ClientError):
     """Generic error when polling for authorization fails.
 
-    This might be caused by either timeout (`exhausted` will be non-empty)
+    This might be caused by either timeout (`waiting` will be non-empty)
     or by some authorization being invalid.
 
-    :ivar exhausted: Set of `.AuthorizationResource` that didn't finish
-        within max allowed attempts.
+    :ivar waiting: Priority queue with `datetime.datatime` (based on
+        ``Retry-After``) as key, and original `.AuthorizationResource`
+        as value.
     :ivar updated: Mapping from original `.AuthorizationResource`
         to the most recently updated one
 
     """
-    def __init__(self, exhausted, updated):
-        self.exhausted = exhausted
+    def __init__(self, waiting, updated):
+        self.waiting = waiting
         self.updated = updated
         super(PollError, self).__init__()
 
     @property
     def timeout(self):
         """Was the error caused by timeout?"""
-        return bool(self.exhausted)
+        return bool(self.waiting)
 
     def __repr__(self):
-        return '{0}(exhausted={1!r}, updated={2!r})'.format(
-            self.__class__.__name__, self.exhausted, self.updated)
+        return '{0}(waiting={1!r}, updated={2!r})'.format(
+            self.__class__.__name__, self.waiting, self.updated)

acme/acme/errors_test.py

@@ -1,4 +1,5 @@
 """Tests for acme.errors."""
+import datetime
 import unittest
 
 import mock
@@ -35,9 +36,9 @@ class PollErrorTest(unittest.TestCase):
     def setUp(self):
         from acme.errors import PollError
         self.timeout = PollError(
-            exhausted=set([mock.sentinel.AR]),
+            waiting=[(datetime.datetime(2015, 11, 29), mock.sentinel.AR)],
             updated={})
-        self.invalid = PollError(exhausted=set(), updated={
+        self.invalid = PollError(waiting=[], updated={
             mock.sentinel.AR: mock.sentinel.AR2})
 
     def test_timeout(self):
@@ -45,7 +46,7 @@ class PollErrorTest(unittest.TestCase):
         self.assertFalse(self.invalid.timeout)
 
     def test_repr(self):
-        self.assertEqual('PollError(exhausted=set([]), updated={sentinel.AR: '
+        self.assertEqual('PollError(waiting=[], updated={sentinel.AR: '
                          'sentinel.AR2})', repr(self.invalid))