[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:  needs_revision
 Priority:  normal           |      Milestone:                
Component:  Stem             |        Version:                
 Keywords:  controller easy  |         Parent:                
   Points:                   |   Actualpoints:                
-----------------------------+----------------------------------------------
Changes (by atagar):

  * status:  needs_review => needs_revision


Comment:

 Looks better! I tend to be very picking during code reviews so it isn't
 unusual for patches to take several iterations. ;)

 {{{
 -      cache_key = "getinfo.%s" % param.lower()
 +    reply = self._get_cache(params, "getinfo")
 }}}

 We seem to be losing case insensitivity for these cache lookups (so
 get_info('version') followed by get_info('VERSION') won't be a cache hit).

 {{{
 +      else:
 +        cache = self._get_cache(["version"])
 +        if cache:
 +          version = cache["version"]
 +        else:
 +          version = stem.version.Version(self.get_info("version"))
 +          self._set_cache({"version": version})

 -      return self._request_cache["version"]
 +      return version
 }}}

 Looks good, just another spacing nit-pick. ;)

 Also, this could be a little better without the 'cache' temporary
 parameter...

 {{{
 else:
   version = self._get_cache(["version"]).get("version", None)

   if not version:
     version = stem.version.Version(self.get_info("version"))
     self._set_cache({"version": version})

   return version
 }}}

 {{{
 -        if not "exit_policy" in self._request_cache:
 +        config_policy = self._get_cache(["exit_policy"])
 +        if not "exit_policy" in config_policy:
            policy = []

            if self.get_conf("ExitPolicyRejectPrivate") == "1":
 @@ -872,10 +879,10 @@ class Controller(BaseController):
              policy += policy_line.split(",")

            policy += self.get_info("exit-policy/default").split(",")
 +          config_policy = stem.exit_policy.get_config_policy(policy)
 +          self._set_cache({"exit_policy": config_policy})

 -          self._request_cache["exit_policy"] =
 stem.exit_policy.get_config_policy(policy)
 -
 -        return self._request_cache["exit_policy"]
 +        return config_policy
 }}}

 It looks to me like when you have a cache hit you're returning a dict of
 {"exit_policy": exit_policy} rather than the exiting policy itself. Maybe
 our tests need to be expanded to cover this. :/

 Between this and the get_version() cache usage I'm starting to wonder if
 we actually want two methods for querying our cache, one for looking up a
 single key and another for looking up multiple. Thoughts?

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