From 2e7fb627e215f4cd93a225f17c0a33d6479abae9 Mon Sep 17 00:00:00 2001 From: shimura233 <2763221922@qq.com> Date: Thu, 11 Apr 2024 21:00:10 +0800 Subject: [PATCH] check whether the object got from the map is an instance of ServiceMethod(see: pr#4114) --- .../src/test/java/retrofit2/RetrofitTest.java | 122 ++++++++++++++++++ .../src/main/java/retrofit2/Retrofit.java | 47 ++++--- 2 files changed, 148 insertions(+), 21 deletions(-) diff --git a/retrofit/java-test/src/test/java/retrofit2/RetrofitTest.java b/retrofit/java-test/src/test/java/retrofit2/RetrofitTest.java index 5bd2c26138..91af6d1725 100644 --- a/retrofit/java-test/src/test/java/retrofit2/RetrofitTest.java +++ b/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 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 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 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 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); + } } diff --git a/retrofit/src/main/java/retrofit2/Retrofit.java b/retrofit/src/main/java/retrofit2/Retrofit.java index 9e5a5bd561..b47396bd2e 100644 --- a/retrofit/src/main/java/retrofit2/Retrofit.java +++ b/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;