Lazy Initialization for ReplaceIfExceptionMatchingProxy (#4669)

* Lazy initialization

* Add generated changelog entries

* Add generated changelog entries

* test

* Remove volatile read

* Refactor memoization to a factory

* oops

* Fix checkstyle

atlasdb-commons/src/main/java/com/palantir/common/proxy/ReplaceIfExceptionMatchingProxy.java

@@ -19,12 +19,16 @@
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
 import java.lang.reflect.Proxy;
+import java.time.Duration;
+import java.util.concurrent.TimeUnit;
 import java.util.function.Predicate;
 import java.util.function.Supplier;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Suppliers;
 import com.google.common.reflect.AbstractInvocationHandler;
 
 public final class ReplaceIfExceptionMatchingProxy<T> extends AbstractInvocationHandler {
@@ -36,35 +40,59 @@ public final class ReplaceIfExceptionMatchingProxy<T> extends AbstractInvocation
 
     private ReplaceIfExceptionMatchingProxy(Supplier<T> delegateFactory, Predicate<Throwable> shouldReplace) {
         this.delegateFactory = delegateFactory;
-        this.delegate = delegateFactory.get();
         this.shouldReplace = shouldReplace;
     }
 
     @Override
     protected Object handleInvocation(Object proxy, Method method, Object[] args) throws Throwable {
         try {
-            return method.invoke(delegate, args);
+            return method.invoke(getAndPossiblyInitializeDelegate(), args);
         } catch (InvocationTargetException e) {
             Throwable cause = e.getCause();
             replaceIfNecessary(cause);
             throw cause;
         }
     }
 
+    private T getAndPossiblyInitializeDelegate() {
+        T perceivedDelegate = delegate;
+        if (perceivedDelegate == null) {
+            synchronized (this) {
+                perceivedDelegate = delegate;
+                if (perceivedDelegate == null) {
+                    perceivedDelegate = delegate = delegateFactory.get();
+                }
+            }
+        }
+        return perceivedDelegate;
+    }
+
     private void replaceIfNecessary(Throwable thrown) {
         if (shouldReplace.test(thrown)) {
             synchronized (this) {
                 T replacement = delegateFactory.get();
                 if (delegate != replacement) {
                     log.info("Replacing underlying proxy due to thrown exception", thrown);
-                    delegate = delegateFactory.get();
+                    delegate = replacement;
                 }
             }
         }
     }
 
+    public static <T> T create(
+            Class<T> interfaceClass,
+            Supplier<T> delegate,
+            Duration minCreationInterval,
+            Predicate<Throwable> shouldReplace) {
+        return newProxyInstance(
+                interfaceClass,
+                Suppliers.memoizeWithExpiration(delegate::get, minCreationInterval.toMillis(), TimeUnit.MILLISECONDS),
+                shouldReplace);
+    }
+
     @SuppressWarnings("unchecked")
-    public static <T> T newProxyInstance(
+    @VisibleForTesting
+    static <T> T newProxyInstance(
             Class<T> interfaceClass,
             Supplier<T> delegate,
             Predicate<Throwable> shouldReplace) {

atlasdb-commons/src/test/java/com/palantir/common/proxy/ReplaceIfExceptionMatchingProxyTest.java

@@ -19,6 +19,7 @@
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
@@ -45,6 +46,15 @@ public void before() {
         when(supplier.get()).thenReturn(delegate);
     }
 
+    @Test
+    public void lazilyInitialized() {
+        TestInterface iface = ReplaceIfExceptionMatchingProxy.newProxyInstance(
+                TestInterface.class, supplier, _thrown -> true);
+        verify(supplier, never()).get();
+        iface.doSomething();
+        verify(supplier, times(1)).get();
+    }
+
     @Test
     public void testExceptionMatching() {
         RuntimeException exception = new RuntimeException();

atlasdb-config/src/main/java/com/palantir/atlasdb/http/AtlasDbHttpClients.java

@@ -16,12 +16,11 @@
 package com.palantir.atlasdb.http;
 
 import java.net.SocketTimeoutException;
+import java.time.Duration;
 import java.util.Optional;
-import java.util.concurrent.TimeUnit;
 import java.util.function.Supplier;
 
 import com.google.common.annotations.VisibleForTesting;
-import com.google.common.base.Suppliers;
 import com.palantir.atlasdb.config.AuxiliaryRemotingParameters;
 import com.palantir.atlasdb.config.ServerListConfig;
 import com.palantir.atlasdb.http.v2.ConjureJavaRuntimeTargetFactory;
@@ -108,9 +107,10 @@ private static <T> T instrument(
      * 2. At most once every 20 minutes
      */
     private static <T> T wrapWithOkHttpBugHandling(Class<T> type, Supplier<T> supplier) {
-        return ReplaceIfExceptionMatchingProxy.newProxyInstance(
+        return ReplaceIfExceptionMatchingProxy.create(
                 type,
-                Suppliers.memoizeWithExpiration(supplier::get, 20, TimeUnit.MINUTES),
+                supplier,
+                Duration.ofMinutes(20),
                 AtlasDbHttpClients::isPossiblyOkHttpTimeoutBug);
     }
 

changelog/@unreleased/pr-4669.v2.yml

@@ -0,0 +1,7 @@
+type: fix
+fix:
+  description: '`ReplaceIfExceptionMatchingProxy` is now lazy, allowing users that
+    manipulate static remoting-layer state to (generally) have static proxies again.
+    Previously these might be initialised before static state changes have gone through.'
+  links:
+  - https://github.com/palantir/atlasdb/pull/4669