[tor-bugs] #8607 [Stem]: Controller's cache isn't thread safe

[Tor Bug Tracker & Wiki]

#8607: Controller's cache isn't thread safe
-----------------------+----------------------------------------------------
    Reporter:  atagar  |       Owner:  atagar         
        Type:  defect  |      Status:  closed         
    Priority:  normal  |   Milestone:                 
   Component:  Stem    |     Version:                 
  Resolution:  fixed   |    Keywords:  controller easy
      Parent:          |      Points:                 
Actualpoints:          |  
-----------------------+----------------------------------------------------
Changes (by atagar):

  * status:  needs_review => closed
  * resolution:  => fixed


Comment:

 Thanks Akshit! Patch pushed with some revisions...

 > reply = {}

 Unnecessary, "reply = {}" was already assigned above.

 > for key in cached_results.keys():

 When you iterate over a dict it's by the keys so there's no need to call
 ".keys()" here.

 {{{
 >>> foo = {'name': 'damian', 'job': 'software engineer'}
 >>> for field in foo:
 ...   print field
 ...
 job
 name
 }}}

 > user_expected_key = _case_insensitive_lookup(params, key, key)

 We know that params has the key. If it doesn't then that probably
 indicates a stem bug so it would be good for this to raise an exception so
 we can more easily discover it.


 The get_conf_map() change puzzles me a bit...

 {{{
 -        for key, value in response.entries.items():
 -          self._request_cache["getconf.%s" % key.lower()] = value
 +        to_cache = {}
 +        for param, value in response.entries.items():
 +          param = param.lower()

 +          if isinstance(value, (bytes, unicode)):
 +            value = [value]
 +          to_cache[param] = value
 +
 +        self._set_cache(to_cache, "getconf")
 }}}

 In the old version it caches the response.entries values while in the new
 version it assumes that response.entries could have either list or str
 values and normalizes as lists. GetConfResponse's pydocs say that the
 values are always lists and by my read of the code that certainly seems to
 be the case. Narrowed this down to just...

 {{{
 if self.is_caching_enabled():
   to_cache = dict((k.lower(), v) for k, v in response.entries.items())
   self._set_cache(to_cache, "getconf")
 }}}

 > if key.lower() == "exitpolicy" and "exit_policy" in self._request_cache:
 >   del self._request_cache["exit_policy"]

 Moving this to _set_cache() seems unsafe to me. This means that if we,
 say, call 'GETINFO exitpolicy' it'll wipe our 'exit_policy' cache entry.
 Moved this back up to where we call SETCONF.

 The _confchanged_listener also didn't account for the new thread safe
 methods. The new methods make it a lot cleaner. :)

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/8607#comment:13>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online