Opened 6 years ago

Closed 6 years ago

#5638 defect closed fixed (fixed)

twisted.names.cache.CacheResolver never times out data it is initialized with

Reported by: Itamar Turner-Trauring Owned by: Itamar Turner-Trauring
Priority: normal Milestone:
Component: names Keywords:
Cc: Branch: branches/cacheresolver-init-5638
branch-diff, diff-cov, branch-cov, buildbot
Author: itamarst, acidfoo


The CacheResolver internal cache can be initialized with beginning values. Unlike cache entries added later, these initial entries will never be timed out or removed.

Attachments (1)

expire-init.diff (3.5 KB) - added by Andrew Snowden 6 years ago.

Download all attachments as: .zip

Change History (5)

Changed 6 years ago by Andrew Snowden

Attachment: expire-init.diff added

comment:1 Changed 6 years ago by Andrew Snowden

Author: acidfoo
Keywords: review added

Changed the constructor to pass the initial entries through cacheResult instead of just assigning the dictionary.

The original unit test (test_lookup) passes in an empty record ([], [], []) which would originally crash the cacheResult function. I wasn't sure if I could just remove the old test, so I made it assume a TTL of 0 when there are no entries in the payload.

comment:2 Changed 6 years ago by Itamar Turner-Trauring

Keywords: review removed
Owner: set to Itamar Turner-Trauring

Thanks for the patch, looks good, with two minor issues:

  1. Extra whitespace at end of lines.
  2. A comment explaining why time elapsed of 40 is good enough to expire the record in the new test would be helpful.

I didn't realise the interface involves passing in absolute timestamps, which is pretty terrible. So I also suggest deprecating passing in cache to the constructor. That can be done in a separate ticket, though. So unless I hear from you this weekend with new patch, I will just commit this one doing the above minor tweaks myself, and open a new ticket for the deprecation.

comment:3 Changed 6 years ago by itamarst

Author: acidfooitamarst, acidfoo
Branch: branches/cacheresolver-init-5638

(In [34351]) Branching to 'cacheresolver-init-5638'

comment:4 Changed 6 years ago by itamarst

Resolution: fixed
Status: newclosed

(In [34359]) Merge cacheresolver-init-5638: Entries added to CacheResolver's init are now timed out.

Author: acidfoo Review: itamar Fixes: #5638

Fix issue where entries added via CacheResolver's constructor were never timed out.

Note: See TracTickets for help on using tickets.