[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #25821 [Core Tor/Stem]: Stem getconf cache doesn't clear for CONF_CHANGED events; probably should set value

#25821: Stem getconf cache doesn't clear for CONF_CHANGED events; probably should
set value
---------------------------+------------------------------
 Reporter:  dmr            |          Owner:  dmr
     Type:  defect         |         Status:  needs_review
 Priority:  Medium         |      Milestone:
Component:  Core Tor/Stem  |        Version:
 Severity:  Normal         |     Resolution:
 Keywords:                 |  Actual Points:
Parent ID:                 |         Points:
 Reviewer:  atagar         |        Sponsor:
---------------------------+------------------------------
Changes (by dmr):

 * status:  assigned => needs_review


Comment:

 Ok, pushed changes; please review :). And sorry for the confusion before!
 I thought I'd assign you as a reviewer before the `needs_review` step.

 == `git request-pull` output
 I'm planning to submit patch notifications in this boilerplate format now,
 to reduce overhead for prepping. Let me know if I should change it!
 `git request-pull master https://github.com/dmr-x/stem 25821-getconf-
 cache`:
 {{{
 The following changes since commit
 38ed5bca90ad6a71b9e45d8916030ea972513b21:

   Reword changelog description of recent CONF_CHANGED fix (2018-05-17
 10:29:15 -0700)

 are available in the git repository at:

   https://github.com/dmr-x/stem 25821-getconf-cache

 for you to fetch changes up to f03f001d4b412d597e766fae4734b863ce4c938d:

   Changelog entry for configuration-changing cache improvements (#25821)
 (2018-05-18 17:22:27 +0000)

 ----------------------------------------------------------------
 Me (6):
       Refactor dropping uncacheable GETCONF params into _set_cache()
       Refactor cache invalidation resulting from config change into new
 method
       Fix bug in cache invalidation for CONF_CHANGED
       Save value rather than invalidate getconf cache in CONF_CHANGED
 events
       Drop cache values for set_options() params instead of setting cache
       Changelog entry for configuration-changing cache improvements
 (#25821)

  docs/change_log.rst |  1 +
  stem/control.py     | 71
 ++++++++++++++++++++++++++++++++++++++++++++---------------------------
  2 files changed, 45 insertions(+), 27 deletions(-)
 }}}

 == Commits background/discussion
 Here's a bit more background on the commits; some refactors in there, that
 also probably fix bugs.

 ==== Refactor 1: into `_set_cache()` - commit
 `9e517998c59a437e7050ebfb685066d249abb21e`
 I did this refactor because I wanted new code to be DRY and not rely on
 every implementer having to remember to include it.
 It's a bit unclean - `_set_cache()` previously was namespace-agnostic and
 now has namespace-specific behavior, but I think it's the best overall
 solution.

 I think this refactor //also// fixed a bug:
 `set_options()` calls `self._set_cache(to_cache, 'getconf')` without
 removing the `UNCACHEABLE_GETCONF_PARAMS` from `to_cache` first.
 So I believe moving the code here **fixed a subtle bug in that case.**

 But the aforementioned code in `set_options()` has been removed from
 another commit, so it's //moot// now.

 ==== Refactor 2: into new `_confchanged_cache_invalidation()` - commit
 `fdd34f81b986fdb21e782993ca380d5e65036494`
 Upon reviewing `set_options()`, realized that there are a lot of cache-
 invalidation things that occur when a config option changes.
 As far as I can tell, these are all applicable to our
 `_confchanged_listener()` too.

 So this "refactor" **probably fixes a few bugs, too**.

 ==== Bug fix - intermediate commit:
 `17782bc44f9afbb7dea49085afad29a936b7a1a3`
 This fixes a bug, but the code gets replaced thereafter. Separated out
 just for posterity.

 ==== Use `change` and `unset` attributes - commit
 `fa1614d20ab57c57e55fcbdb39bece229026bc2b`
 The bulk of the functional work - now `CONF_CHANGED` should work well for
 single- or multiple-controller cases.

 ==== Drop cache, set_options() - commit
 `1eb527579d83ac6d2bc5c15eceeaabe0fa5f927b`
 It also seems important to drop existing cache values to reduce the timing
 window for race conditions, in `set_options()`.
 **Race conditions still can exist; discussed later.**

 ==== Changelog only - commit `f03f001d4b412d597e766fae4734b863ce4c938d`
 //No further explanation needed.//

 == Other notes
 Some other notes about the changes.

 ==== Lack of testing
 Overall I didn't get to writing good tests here. I'd like to use some of
 the test code from #25423 that never got merged in. Just didn't get a
 chance to do that.
 The cache overall doesn't have much in the way of testing.

 ==== Potential for race conditions
 There's still a lot of potential here for race conditions. //(I don't
 think we can fully eliminate that, by the very nature of the controlport
 architecture.)//

 I haven't fully thought about it, but a case that pops out to me is non-
 atomicity of values because of the caching layer.
 Ideally we should change the API to allow for consumers to request that
 calling `get_conf_map()` retrieves all values fresh, so that the set is
 atomic - for consumers that need/care about that. Perhaps as
 `get_conf_map(self, params, default = UNDEFINED, multiple = True,
 force_request=False)`.

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

_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs