Merge pull request #13 from github/dyn-session-create-not-thread-safe

DynectSession creation isn't thread-safe so we need to lock around it

octodns/provider/dyn.py

@@ -14,7 +14,7 @@ from dyn.tm.services.dsf import DSFARecord, DSFAAAARecord, DSFFailoverChain, \
 from dyn.tm.session import DynectSession
 from dyn.tm.zones import Zone as DynZone
 from logging import getLogger
-from threading import local
+from threading import Lock
 from uuid import uuid4
 
 from ..record import Record
@@ -96,6 +96,15 @@ class DynProvider(BaseProvider):
         # Whether or not to support TrafficDirectors and enable GeoDNS
         # (optional, default is false)
         traffic_directors_enabled: true
+
+    Note: due to the way dyn.tm.session.DynectSession is managing things we can
+    only really have a single DynProvider configured. When you create a
+    DynectSession it's stored in a thread-local singleton. You don't invoke
+    methods on this session or a client that holds on to it. The client
+    libraries grab their per-thread session by accessing the singleton through
+    DynectSession.get_session(). That fundamentally doesn't support having more
+    than one account active at a time. See DynProvider._check_dyn_sess for some
+    related bits.
     '''
     RECORDS_TO_TYPE = {
         'a_records': 'A',
@@ -135,7 +144,7 @@ class DynProvider(BaseProvider):
         'AN': 17,  # Continental Antartica
     }
 
-    _thread_local = local()
+    _sess_create_lock = Lock()
 
     def __init__(self, id, customer, username, password,
                  traffic_directors_enabled=False, *args, **kwargs):
@@ -159,21 +168,22 @@ class DynProvider(BaseProvider):
         return self.traffic_directors_enabled
 
     def _check_dyn_sess(self):
-        try:
-            DynProvider._thread_local.dyn_sess
-        except AttributeError:
-            self.log.debug('_check_dyn_sess: creating')
-            # Dynect's client is odd, you create a session object, but don't
-            # use it for anything. It just makes the other objects work behind
-            # the scences. :-( That probably means we can only support a single
-            # set of dynect creds, so no split accounts. They're also per
-            # thread so we need to create one per thread. I originally tried
-            # calling DynectSession.get_session to see if there was one and
-            # creating if not, but that was always returning None, so now I'm
-            # manually creating them once per-thread. I'd imagine this could be
-            # figured out, but ...
-            DynectSession(self.customer, self.username, self.password)
-            DynProvider._thread_local.dyn_sess = True
+        # We don't have to worry about locking for the check since the
+        # underlying pieces are pre-thread. We can check to see if this thread
+        # has a session and if so we're good to go.
+        if DynectSession.get_session() is None:
+            # We need to create a new session for this thread and DynectSession
+            # creation is not thread-safe so we have to do the locking. If we
+            # don't and multiple sessions start creattion before the the first
+            # has finished (long time b/c it makes http calls) the subsequent
+            # creates will blow away DynectSession._instances, potentially
+            # multiple times if there are multiple creates in flight. Only the
+            # last of these initial concurrent creates will exist in
+            # DynectSession._instances dict and the others will be lost. When
+            # this thread later tries to make api calls there won't be an
+            # accessible session available for it to use.
+            with self._sess_create_lock:
+                DynectSession(self.customer, self.username, self.password)
 
     def _data_for_A(self, _type, records):
         return {
@@ -659,6 +669,8 @@ class DynProvider(BaseProvider):
         self.log.debug('_apply: zone=%s, len(changes)=%d', desired.name,
                        len(changes))
 
+        self._check_dyn_sess()
+
         dyn_zone = _CachingDynZone.get(desired.name[:-1], create=True)
 
         if self.traffic_directors_enabled: