Merge pull request square#1639 from square/jw/propagate-executor

Propagate callback executor even when not explicitly specified.

retrofit/src/main/java/retrofit2/Platform.java

@@ -53,6 +53,10 @@ private static Platform findPlatform() {
     return new Platform();
   }
 
+  Executor defaultCallbackExecutor() {
+    return null;
+  }
+
   CallAdapter.Factory defaultCallAdapterFactory(Executor callbackExecutor) {
     if (callbackExecutor != null) {
       return new ExecutorCallAdapterFactory(callbackExecutor);
@@ -89,10 +93,11 @@ static class Java8 extends Platform {
   }
 
   static class Android extends Platform {
+    @Override public Executor defaultCallbackExecutor() {
+      return new MainThreadExecutor();
+    }
+
     @Override CallAdapter.Factory defaultCallAdapterFactory(Executor callbackExecutor) {
-      if (callbackExecutor == null) {
-        callbackExecutor = new MainThreadExecutor();
-      }
       return new ExecutorCallAdapterFactory(callbackExecutor);
     }
 
@@ -106,10 +111,11 @@ static class MainThreadExecutor implements Executor {
   }
 
   static class IOS extends Platform {
+    @Override public Executor defaultCallbackExecutor() {
+      return new MainThreadExecutor();
+    }
+
     @Override CallAdapter.Factory defaultCallAdapterFactory(Executor callbackExecutor) {
-      if (callbackExecutor == null) {
-        callbackExecutor = new MainThreadExecutor();
-      }
       return new ExecutorCallAdapterFactory(callbackExecutor);
     }
 

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

@@ -358,7 +358,10 @@ public <T> Converter<T, String> stringConverter(Type type, Annotation[] annotati
     return (Converter<T, String>) BuiltInConverters.ToStringConverter.INSTANCE;
   }
 
-  /** The executor used for {@link Callback} methods on a {@link Call}. */
+  /**
+   * The executor used for {@link Callback} methods on a {@link Call}. This may be {@code null},
+   * in which case callbacks should be made synchronously on the background thread.
+   */
   public Executor callbackExecutor() {
     return callbackExecutor;
   }
@@ -370,19 +373,25 @@ public Executor callbackExecutor() {
    * are optional.
    */
   public static final class Builder {
+    private Platform platform;
     private okhttp3.Call.Factory callFactory;
     private BaseUrl baseUrl;
     private List<Converter.Factory> converterFactories = new ArrayList<>();
     private List<CallAdapter.Factory> adapterFactories = new ArrayList<>();
     private Executor callbackExecutor;
     private boolean validateEagerly;
 
-    public Builder() {
+    Builder(Platform platform) {
+      this.platform = platform;
       // Add the built-in converter factory first. This prevents overriding its behavior but also
       // ensures correct behavior when using converters that consume all types.
       converterFactories.add(new BuiltInConverters());
     }
 
+    public Builder() {
+      this(Platform.get());
+    }
+
     /**
      * The HTTP client used for requests.
      * <p>
@@ -545,9 +554,14 @@ public Retrofit build() {
         callFactory = new OkHttpClient();
       }
 
+      Executor callbackExecutor = this.callbackExecutor;
+      if (callbackExecutor == null) {
+        callbackExecutor = platform.defaultCallbackExecutor();
+      }
+
       // Make a defensive copy of the adapters and add the default Call adapter.
       List<CallAdapter.Factory> adapterFactories = new ArrayList<>(this.adapterFactories);
-      adapterFactories.add(Platform.get().defaultCallAdapterFactory(callbackExecutor));
+      adapterFactories.add(platform.defaultCallAdapterFactory(callbackExecutor));
 
       // Make a defensive copy of the converters.
       List<Converter.Factory> converterFactories = new ArrayList<>(this.converterFactories);

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

@@ -11,6 +11,7 @@
 import java.util.Set;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.Executor;
+import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicBoolean;
@@ -1156,13 +1157,26 @@ public CallAdapter<?> get(Type returnType, Annotation[] annotations, Retrofit re
     }
   }
 
-  @Test public void callbackExecutorNoDefault() {
+  @Test public void callbackExecutorPropagatesDefaultJvm() {
     Retrofit retrofit = new Retrofit.Builder()
         .baseUrl("http://example.com/")
         .build();
     assertThat(retrofit.callbackExecutor()).isNull();
   }
 
+  @Test public void callbackExecutorPropagatesDefaultAndroid() {
+    final Executor executor = Executors.newSingleThreadExecutor();
+    Platform platform = new Platform() {
+      @Override Executor defaultCallbackExecutor() {
+        return executor;
+      }
+    };
+    Retrofit retrofit = new Retrofit.Builder(platform)
+        .baseUrl("http://example.com/")
+        .build();
+    assertThat(retrofit.callbackExecutor()).isSameAs(executor);
+  }
+
   @Test public void callbackExecutorPropagated() {
     Executor executor = mock(Executor.class);
     Retrofit retrofit = new Retrofit.Builder()