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

acme/acme/client.py

@@ -1,4 +1,5 @@
 """ACME client API."""
+import collections
 import datetime
 import heapq
 import logging
@@ -334,8 +335,9 @@ 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 before
+        :param int max_attempts: Maximum number of attempts (per
-            `PollError` with non-empty ``waiting`` is raised.
+            authorization) before `PollError` with non-empty ``waiting``
+            is raised.
 
         :returns: ``(cert, updated_authzrs)`` `tuple` where ``cert`` is
             the issued certificate (`.messages.CertificateResource`),
@@ -349,6 +351,11 @@ 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]
@@ -356,8 +363,7 @@ class Client(object):  # pylint: disable=too-many-instance-attributes
         # recently updated one
         updated = dict((authzr, authzr) for authzr in authzrs)
 
-        while waiting and max_attempts:
+        while waiting:
-            max_attempts -= 1
             # find the smallest Retry-After, and sleep if necessary
             when, authzr = heapq.heappop(waiting)
             now = datetime.datetime.now()
@@ -371,16 +377,20 @@ 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):
-                # push back to the priority queue, with updated retry_after
+                if attempts[authzr] < max_attempts:
-                heapq.heappush(waiting, (self.retry_after(
+                    # push back to the priority queue, with updated retry_after
-                    response, default=mintime), authzr))
+                    heapq.heappush(waiting, (self.retry_after(
+                        response, default=mintime), authzr))
+                else:
+                    exhausted.add(authzr)
 
-        if not max_attempts or any(authzr.body.status == messages.STATUS_INVALID
+        if exhausted or any(authzr.body.status == messages.STATUS_INVALID
-                                   for authzr in six.itervalues(updated)):
+                            for authzr in six.itervalues(updated)):
-            raise errors.PollError(waiting, updated)
+            raise errors.PollError(exhausted, 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,7 +319,10 @@ class ClientTest(unittest.TestCase):
         )
 
         cert, updated_authzrs = self.client.poll_and_request_issuance(
-            csr, authzrs, mintime=mintime)
+            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)))
         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,26 +56,25 @@ class MissingNonce(NonceError):
 class PollError(ClientError):
     """Generic error when polling for authorization fails.
 
-    This might be caused by either timeout (`waiting` will be non-empty)
+    This might be caused by either timeout (`exhausted` will be non-empty)
     or by some authorization being invalid.
 
-    :ivar waiting: Priority queue with `datetime.datatime` (based on
+    :ivar exhausted: Set of `.AuthorizationResource` that didn't finish
-        ``Retry-After``) as key, and original `.AuthorizationResource`
+        within max allowed attempts.
-        as value.
     :ivar updated: Mapping from original `.AuthorizationResource`
         to the most recently updated one
 
     """
-    def __init__(self, waiting, updated):
+    def __init__(self, exhausted, updated):
-        self.waiting = waiting
+        self.exhausted = exhausted
         self.updated = updated
         super(PollError, self).__init__()
 
     @property
     def timeout(self):
         """Was the error caused by timeout?"""
-        return bool(self.waiting)
+        return bool(self.exhausted)
 
     def __repr__(self):
-        return '{0}(waiting={1!r}, updated={2!r})'.format(
+        return '{0}(exhausted={1!r}, updated={2!r})'.format(
-            self.__class__.__name__, self.waiting, self.updated)
+            self.__class__.__name__, self.exhausted, self.updated)

acme/acme/errors_test.py

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