Opened 3 years ago

Closed 3 years ago

#5638 defect closed fixed (fixed)

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

Reported by: itamar Owned by: itamar
Priority: normal Milestone:
Component: names Keywords:
Cc: Branch: branches/cacheresolver-init-5638
(diff, github, buildbot, log)
Author: itamarst, acidfoo Launchpad Bug:

Description

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 acidfoo 3 years ago.

Download all attachments as: .zip

Change History (5)

Changed 3 years ago by acidfoo

comment:1 Changed 3 years ago by acidfoo

  • Author set to 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 3 years ago by itamar

  • Keywords review removed
  • Owner set to itamar

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 3 years ago by itamarst

  • Author changed from acidfoo to itamarst, acidfoo
  • Branch set to branches/cacheresolver-init-5638

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

comment:4 Changed 3 years ago by itamarst

  • Resolution set to fixed
  • Status changed from new to closed

(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.