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;