From 3f7c9af3a94ca56e65e8c609f15ff7efbaaaa65e Mon Sep 17 00:00:00 2001 From: akwizgran Date: Mon, 22 Nov 2021 12:01:51 +0000 Subject: [PATCH] Create the HTTP client lazily and allow it to be garbage collected. --- .../briar/feed/FeedManagerImpl.java | 26 +++++--------- .../briarproject/briar/feed/FeedModule.java | 29 +++++++++++++++ .../briar/feed/WeakSingletonProvider.java | 35 +++++++++++++++++++ .../briar/feed/FeedManagerImplTest.java | 18 +++++++++- 4 files changed, 89 insertions(+), 19 deletions(-) create mode 100644 briar-core/src/main/java/org/briarproject/briar/feed/WeakSingletonProvider.java diff --git a/briar-core/src/main/java/org/briarproject/briar/feed/FeedManagerImpl.java b/briar-core/src/main/java/org/briarproject/briar/feed/FeedManagerImpl.java index ff44c8ed1..a229809b7 100644 --- a/briar-core/src/main/java/org/briarproject/briar/feed/FeedManagerImpl.java +++ b/briar-core/src/main/java/org/briarproject/briar/feed/FeedManagerImpl.java @@ -44,7 +44,6 @@ import java.io.IOException; import java.io.InputStream; import java.security.GeneralSecurityException; import java.util.ArrayList; -import java.util.Collections; import java.util.Comparator; import java.util.Date; import java.util.List; @@ -55,15 +54,13 @@ import java.util.logging.Logger; import javax.annotation.Nullable; import javax.annotation.concurrent.ThreadSafe; import javax.inject.Inject; -import javax.net.SocketFactory; -import okhttp3.Dns; import okhttp3.OkHttpClient; import okhttp3.Request; import okhttp3.Response; import okhttp3.ResponseBody; -import static java.util.concurrent.TimeUnit.MILLISECONDS; +import static java.util.Collections.sort; import static java.util.logging.Level.WARNING; import static org.briarproject.bramble.util.LogUtils.logException; import static org.briarproject.briar.api.blog.BlogConstants.MAX_BLOG_POST_TEXT_LENGTH; @@ -83,8 +80,6 @@ class FeedManagerImpl implements FeedManager, EventListener, OpenDatabaseHook, private static final Logger LOG = Logger.getLogger(FeedManagerImpl.class.getName()); - private static final int CONNECT_TIMEOUT = 60 * 1000; // Milliseconds - private final TaskScheduler scheduler; private final Executor ioExecutor; private final DatabaseComponent db; @@ -94,7 +89,7 @@ class FeedManagerImpl implements FeedManager, EventListener, OpenDatabaseHook, private final BlogPostFactory blogPostFactory; private final FeedFactory feedFactory; private final Clock clock; - private final OkHttpClient client; + private final WeakSingletonProvider httpClientProvider; private final AtomicBoolean fetcherStarted = new AtomicBoolean(false); private volatile boolean torActive = false; @@ -108,9 +103,8 @@ class FeedManagerImpl implements FeedManager, EventListener, OpenDatabaseHook, BlogManager blogManager, BlogPostFactory blogPostFactory, FeedFactory feedFactory, - SocketFactory torSocketFactory, - Clock clock, - Dns noDnsLookups) { + WeakSingletonProvider httpClientProvider, + Clock clock) { this.scheduler = scheduler; this.ioExecutor = ioExecutor; this.db = db; @@ -119,14 +113,8 @@ class FeedManagerImpl implements FeedManager, EventListener, OpenDatabaseHook, this.blogManager = blogManager; this.blogPostFactory = blogPostFactory; this.feedFactory = feedFactory; + this.httpClientProvider = httpClientProvider; this.clock = clock; - // Share an HTTP client instance between all requests - see - // https://medium.com/@leandromazzuquini/if-you-are-using-okhttp-you-should-know-this-61d68e065a2b - client = new OkHttpClient.Builder() - .socketFactory(torSocketFactory) - .dns(noDnsLookups) // Don't make local DNS lookups - .connectTimeout(CONNECT_TIMEOUT, MILLISECONDS) - .build(); } @Override @@ -374,6 +362,7 @@ class FeedManagerImpl implements FeedManager, EventListener, OpenDatabaseHook, .build(); // Execute Request + OkHttpClient client = httpClientProvider.get(); Response response = client.newCall(request).execute(); ResponseBody body = response.body(); if (body != null) return body.byteStream(); @@ -396,7 +385,8 @@ class FeedManagerImpl implements FeedManager, EventListener, OpenDatabaseHook, long lastEntryTime = feed.getLastEntryTime(); Transaction txn = db.startTransaction(false); try { - Collections.sort(entries, getEntryComparator()); + //noinspection Java8ListSort + sort(entries, getEntryComparator()); for (SyndEntry entry : entries) { long entryTime; if (entry.getPublishedDate() != null) { diff --git a/briar-core/src/main/java/org/briarproject/briar/feed/FeedModule.java b/briar-core/src/main/java/org/briarproject/briar/feed/FeedModule.java index d873c0cd6..2936cbd32 100644 --- a/briar-core/src/main/java/org/briarproject/briar/feed/FeedModule.java +++ b/briar-core/src/main/java/org/briarproject/briar/feed/FeedModule.java @@ -5,15 +5,23 @@ import org.briarproject.bramble.api.lifecycle.LifecycleManager; import org.briarproject.briar.api.blog.BlogManager; import org.briarproject.briar.api.feed.FeedManager; +import javax.annotation.Nonnull; import javax.inject.Inject; import javax.inject.Singleton; +import javax.net.SocketFactory; import dagger.Module; import dagger.Provides; +import okhttp3.Dns; +import okhttp3.OkHttpClient; + +import static java.util.concurrent.TimeUnit.MILLISECONDS; @Module public class FeedModule { + private static final int CONNECT_TIMEOUT = 60_000; // Milliseconds + public static class EagerSingletons { @Inject FeedManager feedManager; @@ -35,4 +43,25 @@ public class FeedModule { return feedFactory; } + // Share an HTTP client instance between requests where possible, while + // allowing the client to be garbage-collected between requests. The + // provider keeps a weak reference to the last client instance and reuses + // the instance until it gets garbage-collected. See + // https://medium.com/@leandromazzuquini/if-you-are-using-okhttp-you-should-know-this-61d68e065a2b + @Provides + @Singleton + WeakSingletonProvider provideOkHttpClientProvider( + SocketFactory torSocketFactory, Dns noDnsLookups) { + return new WeakSingletonProvider() { + @Override + @Nonnull + public OkHttpClient createInstance() { + return new OkHttpClient.Builder() + .socketFactory(torSocketFactory) + .dns(noDnsLookups) // Don't make local DNS lookups + .connectTimeout(CONNECT_TIMEOUT, MILLISECONDS) + .build(); + } + }; + } } diff --git a/briar-core/src/main/java/org/briarproject/briar/feed/WeakSingletonProvider.java b/briar-core/src/main/java/org/briarproject/briar/feed/WeakSingletonProvider.java new file mode 100644 index 000000000..cf3b85ffe --- /dev/null +++ b/briar-core/src/main/java/org/briarproject/briar/feed/WeakSingletonProvider.java @@ -0,0 +1,35 @@ +package org.briarproject.briar.feed; + +import org.briarproject.bramble.api.nullsafety.NotNullByDefault; + +import java.lang.ref.WeakReference; + +import javax.annotation.concurrent.GuardedBy; +import javax.inject.Provider; + +/** + * A {@link Provider} that keeps a {@link WeakReference} to the last provided + * instance and provides the same instance again until the instance is garbage + * collected. + */ +@NotNullByDefault +abstract class WeakSingletonProvider implements Provider { + + private final Object lock = new Object(); + @GuardedBy("lock") + private WeakReference ref = new WeakReference<>(null); + + @Override + public T get() { + synchronized (lock) { + T instance = ref.get(); + if (instance == null) { + instance = createInstance(); + ref = new WeakReference<>(instance); + } + return instance; + } + } + + abstract T createInstance(); +} diff --git a/briar-core/src/test/java/org/briarproject/briar/feed/FeedManagerImplTest.java b/briar-core/src/test/java/org/briarproject/briar/feed/FeedManagerImplTest.java index fb94c9b82..36f401e69 100644 --- a/briar-core/src/test/java/org/briarproject/briar/feed/FeedManagerImplTest.java +++ b/briar-core/src/test/java/org/briarproject/briar/feed/FeedManagerImplTest.java @@ -33,10 +33,13 @@ import java.util.Date; import java.util.List; import java.util.concurrent.Executor; +import javax.annotation.Nonnull; import javax.net.SocketFactory; import okhttp3.Dns; +import okhttp3.OkHttpClient; +import static java.util.concurrent.TimeUnit.MILLISECONDS; import static org.briarproject.bramble.test.TestUtils.getGroup; import static org.briarproject.bramble.test.TestUtils.getLocalAuthor; import static org.briarproject.bramble.test.TestUtils.getMessage; @@ -59,6 +62,19 @@ public class FeedManagerImplTest extends BrambleMockTestCase { private final Clock clock = context.mock(Clock.class); private final Dns noDnsLookups = context.mock(Dns.class); + private final OkHttpClient client = new OkHttpClient.Builder() + .socketFactory(SocketFactory.getDefault()) + .dns(noDnsLookups) + .connectTimeout(60_000, MILLISECONDS) + .build(); + private final WeakSingletonProvider httpClientProvider = + new WeakSingletonProvider() { + @Override + @Nonnull + OkHttpClient createInstance() { + return client; + } + }; private final Group localGroup = getGroup(CLIENT_ID, MAJOR_VERSION); private final GroupId localGroupId = localGroup.getId(); private final Group blogGroup = @@ -73,7 +89,7 @@ public class FeedManagerImplTest extends BrambleMockTestCase { private final FeedManagerImpl feedManager = new FeedManagerImpl(scheduler, ioExecutor, db, contactGroupFactory, clientHelper, blogManager, blogPostFactory, feedFactory, - SocketFactory.getDefault(), clock, noDnsLookups); + httpClientProvider, clock); @Test public void testFetchFeedsReturnsEarlyIfTorIsNotActive() {