add secondary address cache

[murali-shris]

- What I did

• added cache for secondary url result from root server

- How I did it

• added SecondaryAddressCache to at_lookup

- How to verify it

• run the unit test secondary_address_cache_test

- Description for the changelog

[murali-shris]

@gkc @VJag The unit test will verify whether getAddress(...) method works. but the test doesn't verify whether the entry is added to the cache. I can expose a public method to retrieve key from cache. But is it advisable to do so for the purpose of testing?

[gkc]

A few comments

• In most cases, callers will be getting the address from here and subsequently opening and authenticating a connection to the secondary. If opening or authenticating the connection fails then the callers will likely want to have the cache refreshed. (Secondary may have moved, or been reset). I'd suggest adding an optional boolean parameter refreshCacheNow to getAddress, which defaults to false
• I suggest there not be an abstract class as well as a concrete impl. Just have the concrete impl and call it SecondaryAddressCache
• I suggest
  • this not be a singleton, but a normal class
  • have AtClientManager construct the instance and make it available in the same way it makes the AtClient, SyncService and NotificationService available
  • make rootDomain and rootPort instance variables, mandatory in the constructor
• I'd suggest add an optional Duration parameter 'cacheFor' to getAddress, which defaults to 1 hour
• Rather than using a delayed Future to remove the entry, I'd suggest instead
  • add a class like

    class _CacheEntry {
      SecondaryAddress address;
      int expiresAt; // millisecondsFromEpoch
      CacheEntry(this.address, this.expiresAt);
    }

  • have the map be a Map<String, _CacheEntry>
  • have _add construct a _CacheEntry(secondaryAddress, DateTime.now().add(expiryDuration)) to add to the map
  • have getAddress, when it retrieves the _CacheEntry from the cache, check if expiresAt < DateTime.now().millisecondsSinceEpoch and if so, try to refresh the address.
  • add a method bool cacheContains(String atSign)
  • add a method int? getCacheExpiryTime(String atSign) which returns the current cache expiry time
  • maybe If refreshing the address fails, keep the existing entry in the cache and return it
    • A better alternative might be to have a bool parameter 'returnCurrentIfRefreshFails' which defaults to false but, if true, will result in the stale value being returned if the refresh fails. If false, an exception would be thrown.
    • For now, it will just throw an exception
• Unit tests should not make any network call, but
  • Right now, the actual lookup is explicitly done by calling AtLookupImpl.findSecondary()
  • This dependency needs to be abstracted out and made injectable via the SecondaryAddressCache constructor
  • I've done this and called it SecondaryAddressFinder. See updated branch.
• Current unit test requires network connection and knowledge of a real at-sign; this is actually a functional test and should probably be root.atsign.wtf and one of the known stable test at-signs cicd1 to cicd8

I've made all of the above changes in the branch; retained the current functional test and added a couple of basic unit tests, but you will need to add some more unit tests to cover the various combinations of currently-un-cached, currently-cached, verifying that the cache expiry time is correct, checking that the cache is attempted to be refreshed if getAddress is called after the cached entry has expired etc. To make this a bit easier I've added the scaffolding needed for mocking the SecondaryAddressFinder using the mocktail package

[murali-shris]

@gkc say i lookup @ alice from bob's client. But after this I never lookup @ alice again. In this case, @ alice should be removed from cache at some point..right?

[gkc]

I don't think there's any need to remove @ alice from the cache; I don't see any downside to leaving it there except for a few bytes of memory.

[gkc]

Nice!

[gkc]

maybe add an assertion that the atsign is not cached before calling cache.getAddress

[murali-shris]

done

[gkc]

Have been wondering more about this. One of the problems we have is when we throw an exception. So I think we should just return the cached value (even if it is stale, it's better than throwing an exception) - and perhaps for visibility, add another field to the CacheEntry like 'lookupException' which would be 'most recent cache refresh fail reason'; would be set when we catch an exception, would be cleared when a lookup succeeds.