Check the Object from the serviceMethodCache again

retrofit/java-test/src/test/java/retrofit2/RetrofitTest.java

@@ -1802,4 +1802,126 @@ public void annotationParsingFailureObservedByWaitingThreads() throws Interrupte
  assertThat(failure3).hasCauseThat().isSameInstanceAs(failure);
  assertThat(fails.get()).isEqualTo(3);
  }
+
+ @Test
+ public void annotationParsingFailureObservedByMultipleWaitingThreads()
+ throws InterruptedException {
+ AtomicInteger fails = new AtomicInteger();
+ CountDownLatch startedParsing = new CountDownLatch(1);
+ CountDownLatch failParsing = new CountDownLatch(1);
+ RuntimeException failure = new RuntimeException("boom!");
+ Retrofit retrofit =
+ new Retrofit.Builder()
+ .baseUrl(server.url("/"))
+ .addConverterFactory(
+ new Converter.Factory() {
+ @Nullable
+ @Override
+ public Converter<ResponseBody, ?> responseBodyConverter(
+ Type type, Annotation[] annotations, Retrofit retrofit) {
+ startedParsing.countDown();
+ try {
+ failParsing.await();
+ // To guarantee that the lock inserted by current thread may stay in the map
+ // at
+ // least 2 seconds to increase the probability that other threads get this
+ // lock.
+ Thread.sleep(2_000);
+ } catch (InterruptedException e) {
+ throw new AssertionError(e);
+ }
+ fails.incrementAndGet();
+ throw failure;
+ }
+ })
+ .build();
+ Annotated service = retrofit.create(Annotated.class);
+
+ AtomicReference<RuntimeException> result1 = new AtomicReference<>();
+ Thread thread1 =
+ new Thread(
+ () -> {
+ try {
+ service.method();
+ } catch (RuntimeException e) {
+ result1.set(e);
+ }
+ });
+ thread1.start();
+
+ // Wait for thread1 to enter the converter. This means it has inserted and taken a lock on
+ // parsing for the method.
+ startedParsing.await();
+
+ CountDownLatch thread2Locked = new CountDownLatch(1);
+ AtomicReference<RuntimeException> result2 = new AtomicReference<>();
+ Thread thread2 =
+ new Thread(
+ () -> {
+ try {
+ thread2Locked.countDown();
+ service.method();
+ } catch (RuntimeException e) {
+ result2.set(e);
+ }
+ });
+ thread2.start();
+ thread2Locked.await();
+ // Wait for thread2 to lock on the shared object inserted by thread1. This should be pretty fast
+ // after the last signal, but we have no way of knowing for sure it happened.
+ Thread.sleep(1_000);
+
+ CountDownLatch thread3Locked = new CountDownLatch(1);
+ AtomicReference<RuntimeException> result3 = new AtomicReference<>();
+ Thread thread3 =
+ new Thread(
+ () -> {
+ try {
+ thread3Locked.countDown();
+ service.method();
+ } catch (RuntimeException e) {
+ result3.set(e);
+ }
+ });
+ thread3.start();
+ thread3Locked.await();
+ // Wait for thread3 to lock on the shared object inserted by thread1. This should be pretty fast
+ // after the last signal, but we have no way of knowing for sure it happened.
+ Thread.sleep(1_000);
+
+ failParsing.countDown();
+ // After 2_000 ms, thread1 failed its parsing and released the lock.
+ // Thread2 and thread3 try to synchronize this lock and put their own locks.
+ // Let's say that thread2 took the lock before thread3, then thread3 taking the lock,
+ // the object thread3 got from the map might be null or a lock inserted by thread2.
+ // If null, thread3 should compete with thread2 to put its own lock and parse the model.
+ // Otherwise, wait for thread2 to finish model parsing(and its doomed failure).
+ thread1.join();
+ thread2.join();
+ thread3.join();
+
+ RuntimeException failure1 = result1.get();
+ assertThat(failure1).isInstanceOf(IllegalArgumentException.class);
+ assertThat(failure1).hasCauseThat().isSameInstanceAs(failure);
+
+ RuntimeException failure2 = result2.get();
+ assertThat(failure2).isInstanceOf(IllegalArgumentException.class);
+ assertThat(failure2).hasCauseThat().isSameInstanceAs(failure);
+
+ RuntimeException failure3 = result3.get();
+ assertThat(failure3).isInstanceOf(IllegalArgumentException.class);
+ assertThat(failure3).hasCauseThat().isSameInstanceAs(failure);
+
+ // Importantly, even though the second and the third threads were locked waiting on the first,
+ // failure of the
+ // first thread caused the second thread to retry parsing. And the failure of the second(or the
+ // third) one
+ // caused the third(or the second) thread to retry parsing.
+ assertThat(fails.get()).isEqualTo(3);
+
+ // Make sure now that all the threads have released the lock, new callers also retry.
+ RuntimeException failure4 = assertThrows(IllegalArgumentException.class, service::method);
+ assertThat(failure4).hasCauseThat().isSameInstanceAs(failure);
+ assertThat(fails.get()).isEqualTo(4);
+ }
 }

retrofit/src/main/java/retrofit2/Retrofit.java

@@ -212,21 +212,26 @@ private void validateServiceInterface(Class<?> service) {
  }
 
  ServiceMethod<?> loadServiceMethod(Class<?> service, Method method) {
+ // Note: Once we are minSdk 24 this whole method can be replaced by computeIfAbsent.
+ Object lookup = serviceMethodCache.get(method);
+ if (lookup instanceof ServiceMethod<?>) {
+ // Happy path: method is already parsed into the model.
+ return (ServiceMethod<?>) lookup;
+ }
+ // Invariant: lookup is null or someone else's lock
  while (true) {
- // Note: Once we are minSdk 24 this whole method can be replaced by computeIfAbsent.
- Object lookup = serviceMethodCache.get(method);
-
- if (lookup instanceof ServiceMethod<?>) {
- // Happy path: method is already parsed into the model.
- return (ServiceMethod<?>) lookup;
- }
-
  if (lookup == null) {
  // Map does not contain any value. Try to put in a lock for this method. We MUST synchronize
  // on the lock before it is visible to others via the map to signal we are doing the work.
  Object lock = new Object();
  synchronized (lock) {
  lookup = serviceMethodCache.putIfAbsent(method, lock);
+ // Other threads may have successfully parsed the model and updated the map now.
+ // Thus, check whether lookup is a finished model again.
+ if (lookup instanceof ServiceMethod<?>) {
+ // Happy path: method is already parsed into the model.
+ return (ServiceMethod<?>) lookup;
+ }
  if (lookup == null) {
  // On successful lock insertion, perform the work and update the map before releasing.
  // Other threads may be waiting on lock now and will expect the parsed model.
@@ -244,27 +249,27 @@ ServiceMethod<?> loadServiceMethod(Class<?> service, Method method) {
  }
  }
 
- // Either the initial lookup or the attempt to put our lock in the map has returned someone
- // else's lock. This means they are doing the parsing, and will update the map before
- // releasing
+ // Either the initial lookup of this iteration or the attempt to put our lock in the map has
+ // returned someone else's lock.
+ // This means they are doing the parsing, and will update the map before releasing
  // the lock. Once we can take the lock, the map is guaranteed to contain the model or null.
- // Note: There's a chance that our effort to put a lock into the map has actually returned a
- // finished model instead of a lock. In that case this code will perform a pointless lock and
- // redundant lookup in the map of the same instance. This is rare, and ultimately harmless.
- synchronized (lookup) {
- Object result = serviceMethodCache.get(method);
- if (result == null) {
- // The other thread failed its parsing. We will retry (and probably also fail).
- continue;
+ Object lock = lookup;
+ synchronized (lock) {
+ lookup = serviceMethodCache.get(method);
+ if (lookup instanceof ServiceMethod<?>) {
+ return (ServiceMethod<?>) lookup;
  }
- return (ServiceMethod<?>) result;
  }
+ // The thread the lock of which we are taking failed its parsing.
+ // Another thread may have detected this failure and put its lock into the map.
+ // Now, lookup is null or someone else's lock.
+ // We will retry (and probably also fail).
  }
  }
 
  /**
  * The factory used to create {@linkplain okhttp3.Call OkHttp calls} for sending a HTTP requests.
- * Typically an instance of {@link OkHttpClient}.
+ * Typically, an instance of {@link OkHttpClient}.
  */
  public okhttp3.Call.Factory callFactory() {
  return callFactory;