Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 24
feat: Reduce streaming DNS failures with stale-fallback DNS cache#323
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
f990e9e1cd1904ebd37ab4c65601443c6b30a6a23eFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,113 @@ | ||
| package com.launchdarkly.sdk.android; | ||
| import androidx.annotation.VisibleForTesting; | ||
| import com.launchdarkly.logging.LDLogger; | ||
| import java.net.InetAddress; | ||
| import java.net.UnknownHostException; | ||
| import java.util.ArrayList; | ||
| import java.util.Collections; | ||
| import java.util.List; | ||
| import java.util.concurrent.ConcurrentHashMap; | ||
| import okhttp3.Dns; | ||
| /** | ||
| * A DNS resolver that caches successful lookups and falls back to stale cache | ||
| * entries when a fresh resolution fails. This is particularly useful on mobile | ||
| * networks where DNS can be unreliable during network transitions. | ||
| * <p> | ||
| * Although Android API 34+ exposes {@code DnsOptions.StaleDnsOptions} in | ||
| * {@code DnsResolver}, OkHttp's {@code Dns.SYSTEM} uses | ||
| * {@code InetAddress.getAllByName} which does not opt into that mechanism. | ||
| * This class is therefore used on all API levels. | ||
| * <p> | ||
| * Instances of this class are thread-safe and designed to be shared across | ||
| * multiple OkHttpClient instances so that the cache persists even when the | ||
| * HTTP client is recreated (e.g. on EventSource reconnections). | ||
| */ | ||
| final class CachingDns implements Dns { | ||
| @VisibleForTesting | ||
| static final long DEFAULT_TTL_MS = 10 * 60 * 1000; // 10 minutes | ||
| @VisibleForTesting | ||
| static final int MAX_ENTRIES = 30; | ||
| private final Dns delegate; | ||
| private final long ttlMs; | ||
| private final LDLogger logger; | ||
| private final ConcurrentHashMap<String, CacheEntry> cache = new ConcurrentHashMap<>(); | ||
| static final class CacheEntry { | ||
| final List<InetAddress> addresses; | ||
| final long expiresAtMs; | ||
| CacheEntry(List<InetAddress> addresses, long expiresAtMs) { | ||
| this.addresses = Collections.unmodifiableList(new ArrayList<>(addresses)); | ||
| this.expiresAtMs = expiresAtMs; | ||
| } | ||
| boolean isExpired(long nowMs) { | ||
| return nowMs >= expiresAtMs; | ||
| } | ||
| } | ||
| CachingDns(Dns delegate, long ttlMs, LDLogger logger) { | ||
| this.delegate = delegate; | ||
| this.ttlMs = ttlMs; | ||
| this.logger = logger; | ||
| } | ||
| CachingDns(LDLogger logger) { | ||
| this(Dns.SYSTEM, DEFAULT_TTL_MS, logger); | ||
| } | ||
| @Override | ||
| public List<InetAddress> lookup(String hostname) throws UnknownHostException { | ||
| long now = System.currentTimeMillis(); | ||
| CacheEntry entry = cache.get(hostname); | ||
| if (entry != null && !entry.isExpired(now)) { | ||
| return entry.addresses; | ||
| } | ||
| try { | ||
| List<InetAddress> addresses = delegate.lookup(hostname); | ||
| long afterLookup = System.currentTimeMillis(); | ||
| if (cache.size() >= MAX_ENTRIES) { | ||
| evictExpired(afterLookup); | ||
| } | ||
| cache.put(hostname, new CacheEntry(addresses, afterLookup + ttlMs)); | ||
| return addresses; | ||
cursor[bot] marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| } catch (UnknownHostException e) { | ||
| if (entry != null) { | ||
| logger.warn( | ||
| "DNS lookup failed for {}, falling back to cached address (age {}ms)", | ||
| hostname, System.currentTimeMillis() - (entry.expiresAtMs - ttlMs) | ||
| ); | ||
| return entry.addresses; | ||
| } | ||
| throw e; | ||
| } | ||
cursor[bot] marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| } | ||
cursor[bot] marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| private void evictExpired(long now) { | ||
| java.util.Iterator<java.util.Map.Entry<String, CacheEntry>> it = cache.entrySet().iterator(); | ||
| while (it.hasNext()) { | ||
| if (it.next().getValue().isExpired(now)) { | ||
| it.remove(); | ||
| } | ||
| } | ||
| } | ||
| @VisibleForTesting | ||
| int cacheSize() { | ||
| return cache.size(); | ||
| } | ||
| @VisibleForTesting | ||
| CacheEntry getCacheEntry(String hostname) { | ||
| return cache.get(hostname); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -28,6 +28,10 @@ | ||
| import java.io.IOException; | ||
| import java.util.HashMap; | ||
| import java.util.Map; | ||
| import java.util.concurrent.TimeUnit; | ||
| import okhttp3.ConnectionPool; | ||
| import okhttp3.Dns; | ||
| /** | ||
| * This class contains the package-private implementations of component factories and builders whose | ||
| @@ -322,6 +326,27 @@ public PollingDataSourceBuilder pollIntervalMillisNoMinimum(int pollIntervalMill | ||
| static final class StreamingDataSourceBuilderImpl extends StreamingDataSourceBuilder | ||
| implements DiagnosticDescription, DataSourceRequiresFeatureFetcher { | ||
| // Shared across StreamingDataSource instances so that DNS cache and pooled | ||
| // connections survive data-source restarts (context switches, network transitions). | ||
| private volatile CachingDns sharedDns; | ||
| private final ConnectionPool sharedConnectionPool = | ||
| new ConnectionPool(1, 5, TimeUnit.MINUTES); | ||
| private Dns getOrCreateDns(LDLogger logger) { | ||
| CachingDns dns = sharedDns; | ||
| if (dns == null) { | ||
| synchronized (this) { | ||
| dns = sharedDns; | ||
| if (dns == null) { | ||
| dns = new CachingDns(logger); | ||
| sharedDns = dns; | ||
| } | ||
| } | ||
| } | ||
| return dns; | ||
| } | ||
| @Override | ||
| public DataSource build(ClientContext clientContext) { | ||
| // Even though this is called StreamingDataSourceBuilder, it doesn't always create a | ||
| @@ -342,7 +367,9 @@ public DataSource build(ClientContext clientContext) { | ||
| clientContext.getDataSourceUpdateSink(), | ||
| clientContextImpl.getFetcher(), | ||
| initialReconnectDelayMillis, | ||
| streamEvenInBackground | ||
| streamEvenInBackground, | ||
| getOrCreateDns(clientContext.getBaseLogger()), | ||
| sharedConnectionPool | ||
abelonogov-ld marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| ); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -24,6 +24,8 @@ | ||
| import java.net.URI; | ||
| import java.util.concurrent.TimeUnit; | ||
| import okhttp3.ConnectionPool; | ||
| import okhttp3.Dns; | ||
| import okhttp3.OkHttpClient; | ||
| import okhttp3.RequestBody; | ||
| @@ -65,14 +67,18 @@ final class StreamingDataSource implements DataSource { | ||
| private final DiagnosticStore diagnosticStore; | ||
| private long eventSourceStarted; | ||
| private final LDLogger logger; | ||
| private final Dns dns; | ||
| private final ConnectionPool connectionPool; | ||
| StreamingDataSource( | ||
| @NonNull ClientContext clientContext, | ||
| @NonNull LDContext context, | ||
| @NonNull DataSourceUpdateSink dataSourceUpdateSink, | ||
| @NonNull FeatureFetcher fetcher, | ||
| int initialReconnectDelayMillis, | ||
| boolean streamEvenInBackground | ||
| boolean streamEvenInBackground, | ||
| @NonNull Dns dns, | ||
| @NonNull ConnectionPool connectionPool | ||
| ) { | ||
| this.context = context; | ||
| this.dataSourceUpdateSink = dataSourceUpdateSink; | ||
| @@ -85,6 +91,8 @@ final class StreamingDataSource implements DataSource { | ||
| this.streamEvenInBackground = streamEvenInBackground; | ||
| this.diagnosticStore = ClientContextImpl.get(clientContext).getDiagnosticStore(); | ||
| this.logger = clientContext.getBaseLogger(); | ||
| this.dns = dns; | ||
| this.connectionPool = connectionPool; | ||
| } | ||
| public void start(@NonNull Callback<Boolean> resultCallback) { | ||
| @@ -152,6 +160,9 @@ public void onError(Throwable t) { | ||
| public void configure(OkHttpClient.Builder clientBuilder) { | ||
| httpProperties.applyToHttpClientBuilder(clientBuilder); | ||
| clientBuilder.readTimeout(READ_TIMEOUT_MS, TimeUnit.MILLISECONDS); | ||
| clientBuilder.dns(dns); | ||
| clientBuilder.connectionPool(connectionPool); | ||
| clientBuilder.retryOnConnectionFailure(true); | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The streaming data source already follows an exponential backoff at the event source layer. We shouldn't add more try logic as the exponential backoff was specified with agreement from flag delivery to have a known load in the case of cloud outage + recovery. | ||
| } | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,171 @@ | ||
| package com.launchdarkly.sdk.android; | ||
| import static org.junit.Assert.assertEquals; | ||
| import com.launchdarkly.logging.LDLogger; | ||
| import org.junit.Test; | ||
| import java.net.InetAddress; | ||
| import java.net.UnknownHostException; | ||
| import java.util.Collections; | ||
| import java.util.List; | ||
| import java.util.concurrent.atomic.AtomicInteger; | ||
| import okhttp3.Dns; | ||
| public class CachingDnsTest { | ||
| private static final LDLogger logger = LDLogger.none(); | ||
| private static Dns counting(List<InetAddress> result, AtomicInteger counter) { | ||
| return hostname -> { | ||
| counter.incrementAndGet(); | ||
| return result; | ||
| }; | ||
| } | ||
| private static Dns failing() { | ||
| return hostname -> { | ||
| throw new UnknownHostException("simulated DNS failure for " + hostname); | ||
| }; | ||
| } | ||
| @Test | ||
| public void returnsFreshResultFromDelegate() throws Exception { | ||
| InetAddress addr = InetAddress.getByName("127.0.0.1"); | ||
| List<InetAddress> expected = Collections.singletonList(addr); | ||
| AtomicInteger lookups = new AtomicInteger(); | ||
| CachingDns dns = new CachingDns(counting(expected, lookups), 60_000, logger); | ||
| List<InetAddress> result = dns.lookup("example.com"); | ||
| assertEquals(expected, result); | ||
| assertEquals(1, lookups.get()); | ||
| } | ||
| @Test | ||
| public void returnsCachedResultWithinTtl() throws Exception { | ||
| InetAddress addr = InetAddress.getByName("127.0.0.1"); | ||
| List<InetAddress> expected = Collections.singletonList(addr); | ||
| AtomicInteger lookups = new AtomicInteger(); | ||
| CachingDns dns = new CachingDns(counting(expected, lookups), 60_000, logger); | ||
| dns.lookup("example.com"); | ||
| List<InetAddress> result = dns.lookup("example.com"); | ||
| assertEquals(expected, result); | ||
| assertEquals(1, lookups.get()); | ||
| } | ||
| @Test | ||
| public void refreshesAfterTtlExpires() throws Exception { | ||
| InetAddress addr = InetAddress.getByName("127.0.0.1"); | ||
| List<InetAddress> expected = Collections.singletonList(addr); | ||
| AtomicInteger lookups = new AtomicInteger(); | ||
| CachingDns dns = new CachingDns(counting(expected, lookups), 0, logger); | ||
| dns.lookup("example.com"); | ||
| dns.lookup("example.com"); | ||
| assertEquals(2, lookups.get()); | ||
| } | ||
| @Test | ||
| public void fallsBackToStaleCacheOnFailure() throws Exception { | ||
| InetAddress addr = InetAddress.getByName("127.0.0.1"); | ||
| List<InetAddress> cached = Collections.singletonList(addr); | ||
| AtomicInteger lookups = new AtomicInteger(); | ||
| Dns flaky = hostname -> { | ||
| if (lookups.incrementAndGet() == 1) { | ||
| return cached; | ||
| } | ||
| throw new UnknownHostException("simulated failure"); | ||
| }; | ||
| CachingDns dns = new CachingDns(flaky, 0, logger); | ||
| dns.lookup("example.com"); | ||
| List<InetAddress> result = dns.lookup("example.com"); | ||
| assertEquals(cached, result); | ||
| } | ||
| @Test(expected = UnknownHostException.class) | ||
| public void throwsWhenDelegateFailsAndNoCacheExists() throws Exception { | ||
| CachingDns dns = new CachingDns(failing(), 60_000, logger); | ||
| dns.lookup("no-such-host.invalid"); | ||
| } | ||
| @Test | ||
| public void cachesPerHostname() throws Exception { | ||
| InetAddress addr1 = InetAddress.getByName("10.0.0.1"); | ||
| InetAddress addr2 = InetAddress.getByName("10.0.0.2"); | ||
| List<InetAddress> list1 = Collections.singletonList(addr1); | ||
| List<InetAddress> list2 = Collections.singletonList(addr2); | ||
| AtomicInteger lookups = new AtomicInteger(); | ||
| Dns delegate = hostname -> { | ||
| lookups.incrementAndGet(); | ||
| return hostname.equals("a.example.com") ? list1 : list2; | ||
| }; | ||
| CachingDns dns = new CachingDns(delegate, 60_000, logger); | ||
| assertEquals(list1, dns.lookup("a.example.com")); | ||
| assertEquals(list2, dns.lookup("b.example.com")); | ||
| assertEquals(2, lookups.get()); | ||
| assertEquals(list1, dns.lookup("a.example.com")); | ||
| assertEquals(list2, dns.lookup("b.example.com")); | ||
| assertEquals(2, lookups.get()); | ||
| } | ||
| @Test | ||
| public void evictsExpiredEntriesWhenCacheExceedsMax() throws Exception { | ||
| InetAddress addr = InetAddress.getByName("127.0.0.1"); | ||
| List<InetAddress> addrs = Collections.singletonList(addr); | ||
| AtomicInteger lookups = new AtomicInteger(); | ||
| // TTL of 0 means every entry expires immediately | ||
| CachingDns dns = new CachingDns(counting(addrs, lookups), 0, logger); | ||
| // Fill beyond MAX_ENTRIES with distinct hostnames; each previous entry | ||
| // is already expired by the time the next lookup runs. | ||
| for (int i = 0; i <= CachingDns.MAX_ENTRIES; i++) { | ||
| dns.lookup("host-" + i + ".example.com"); | ||
| } | ||
| // The last put should have triggered eviction of all expired entries, | ||
| // leaving only the most recent (non-expired at the instant it was stored). | ||
| assertEquals(1, dns.cacheSize()); | ||
| } | ||
| @Test | ||
| public void retainsNonExpiredEntriesAcrossEviction() throws Exception { | ||
| InetAddress addr = InetAddress.getByName("127.0.0.1"); | ||
| List<InetAddress> addrs = Collections.singletonList(addr); | ||
| AtomicInteger lookups = new AtomicInteger(); | ||
| // Long TTL so nothing expires during the test | ||
| CachingDns dns = new CachingDns(counting(addrs, lookups), 600_000, logger); | ||
| for (int i = 0; i <= CachingDns.MAX_ENTRIES; i++) { | ||
| dns.lookup("host-" + i + ".example.com"); | ||
| } | ||
| // Nothing is expired, so eviction can't remove anything | ||
| assertEquals(CachingDns.MAX_ENTRIES + 1, dns.cacheSize()); | ||
| } | ||
| @Test | ||
| public void cacheEntryRecordsExpiration() { | ||
| InetAddress loopback = InetAddress.getLoopbackAddress(); | ||
| List<InetAddress> addrs = Collections.singletonList(loopback); | ||
| long now = System.currentTimeMillis(); | ||
| CachingDns.CacheEntry entry = new CachingDns.CacheEntry(addrs, now + 5000); | ||
| assertEquals(false, entry.isExpired(now)); | ||
| assertEquals(true, entry.isExpired(now + 5000)); | ||
| assertEquals(true, entry.isExpired(now + 6000)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should only use the cache in cases where the delegate fails to do lookup to minimize the impact of these changes on existing cases. My understanding of this PR is to handle a case that is not handled well today and not to improve the performance of caching in general.
Another reason is this cache has no other signals that can remove elements from it besides time, such as registered IP changes or VPN region change. It is feasible that a delegate could be aware of extra signals, but by using our own cache, we do not give the delegate an opportunity to use its possibly more complex behavior.