From 0beab2895041004e49d1b38fbbf82d825870c732 Mon Sep 17 00:00:00 2001 From: Huzaifa Iftikhar Date: Thu, 3 Sep 2026 02:18:25 +0500 Subject: [PATCH] Replace synchronized in OkHttpCall with atomics The class guarded three fields with the monitor: the lazily created okhttp3.Call, the throwable that creating it threw, and the executed flag. Holding a monitor across call creation pins a carrier thread when the caller is a virtual thread. executed becomes an AtomicBoolean, so the one shot check is a single compareAndSet rather than a read and a write under a lock. rawCall and creationFailure become one AtomicReference holding whichever of the two happened, since exactly one of them does and both have to be remembered for later callers. Creation is no longer serialised: two threads arriving together both create a call and one wins the swap. An unexecuted call holds no connection so discarding the loser costs nothing, which is the tradeoff suggested on the issue. --- .../src/main/java/retrofit2/OkHttpCall.java | 116 ++++++++---------- 1 file changed, 54 insertions(+), 62 deletions(-) diff --git a/retrofit/src/main/java/retrofit2/OkHttpCall.java b/retrofit/src/main/java/retrofit2/OkHttpCall.java index ff67612263..acd070e808 100644 --- a/retrofit/src/main/java/retrofit2/OkHttpCall.java +++ b/retrofit/src/main/java/retrofit2/OkHttpCall.java @@ -19,8 +19,9 @@ import java.io.IOException; import java.util.Objects; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; import javax.annotation.Nullable; -import javax.annotation.concurrent.GuardedBy; import okhttp3.MediaType; import okhttp3.Request; import okhttp3.ResponseBody; @@ -39,14 +40,14 @@ final class OkHttpCall implements Call { private volatile boolean canceled; - @GuardedBy("this") - private @Nullable okhttp3.Call rawCall; - - @GuardedBy("this") // Either a RuntimeException, non-fatal Error, or IOException. - private @Nullable Throwable creationFailure; + /** + * The outcome of creating the underlying call, which happens at most once. Holds either the + * {@link okhttp3.Call} or the {@link Throwable} that creating it threw, since exactly one of + * those happens and both have to be remembered for later callers. + */ + private final AtomicReference callOrFailure = new AtomicReference<>(); - @GuardedBy("this") - private boolean executed; + private final AtomicBoolean executed = new AtomicBoolean(); OkHttpCall( RequestFactory requestFactory, @@ -68,7 +69,7 @@ public OkHttpCall clone() { } @Override - public synchronized Request request() { + public Request request() { try { return getRawCall().request(); } catch (IOException e) { @@ -77,7 +78,7 @@ public synchronized Request request() { } @Override - public synchronized Timeout timeout() { + public Timeout timeout() { try { return getRawCall().timeout(); } catch (IOException e) { @@ -89,29 +90,32 @@ public synchronized Timeout timeout() { * Returns the raw call, initializing it if necessary. Throws if initializing the raw call throws, * or has thrown in previous attempts to create it. */ - @GuardedBy("this") private okhttp3.Call getRawCall() throws IOException { - okhttp3.Call call = rawCall; - if (call != null) return call; - - // Re-throw previous failures if this isn't the first attempt. - if (creationFailure != null) { - if (creationFailure instanceof IOException) { - throw (IOException) creationFailure; - } else if (creationFailure instanceof RuntimeException) { - throw (RuntimeException) creationFailure; - } else { - throw (Error) creationFailure; + Object outcome = callOrFailure.get(); + if (outcome == null) { + // Two threads arriving together will both create a call and only one will be kept. That is + // cheaper than holding a lock for the whole creation, and an unexecuted call holds no + // connection, so discarding the loser costs nothing. + try { + okhttp3.Call created = createRawCall(); + outcome = callOrFailure.compareAndSet(null, created) ? created : callOrFailure.get(); + } catch (RuntimeException | Error | IOException e) { + throwIfFatal(e); // Do not remember a fatal error. + outcome = callOrFailure.compareAndSet(null, e) ? e : callOrFailure.get(); } } - // Create and remember either the success or the failure. - try { - return rawCall = createRawCall(); - } catch (RuntimeException | Error | IOException e) { - throwIfFatal(e); // Do not assign a fatal error to creationFailure. - creationFailure = e; - throw e; + if (outcome instanceof okhttp3.Call) { + return (okhttp3.Call) outcome; + } + // Re-throw the failure that the winning attempt recorded. + Throwable creationFailure = (Throwable) outcome; + if (creationFailure instanceof IOException) { + throw (IOException) creationFailure; + } else if (creationFailure instanceof RuntimeException) { + throw (RuntimeException) creationFailure; + } else { + throw (Error) creationFailure; } } @@ -119,23 +123,17 @@ private okhttp3.Call getRawCall() throws IOException { public void enqueue(final Callback callback) { Objects.requireNonNull(callback, "callback == null"); - okhttp3.Call call; - Throwable failure; - - synchronized (this) { - if (executed) throw new IllegalStateException("Already executed."); - executed = true; - - call = rawCall; - failure = creationFailure; - if (call == null && failure == null) { - try { - call = rawCall = createRawCall(); - } catch (Throwable t) { - throwIfFatal(t); - failure = creationFailure = t; - } - } + if (!executed.compareAndSet(false, true)) { + throw new IllegalStateException("Already executed."); + } + + okhttp3.Call call = null; + Throwable failure = null; + try { + call = getRawCall(); + } catch (RuntimeException | Error | IOException t) { + throwIfFatal(t); + failure = t; } if (failure != null) { @@ -185,20 +183,18 @@ private void callFailure(Throwable e) { } @Override - public synchronized boolean isExecuted() { - return executed; + public boolean isExecuted() { + return executed.get(); } @Override public Response execute() throws IOException { okhttp3.Call call; - synchronized (this) { - if (executed) throw new IllegalStateException("Already executed."); - executed = true; - - call = getRawCall(); + if (!executed.compareAndSet(false, true)) { + throw new IllegalStateException("Already executed."); } + call = getRawCall(); if (canceled) { call.cancel(); @@ -257,12 +253,9 @@ Response parseResponse(okhttp3.Response rawResponse) throws IOException { public void cancel() { canceled = true; - okhttp3.Call call; - synchronized (this) { - call = rawCall; - } - if (call != null) { - call.cancel(); + Object outcome = callOrFailure.get(); + if (outcome instanceof okhttp3.Call) { + ((okhttp3.Call) outcome).cancel(); } } @@ -271,9 +264,8 @@ public boolean isCanceled() { if (canceled) { return true; } - synchronized (this) { - return rawCall != null && rawCall.isCanceled(); - } + Object outcome = callOrFailure.get(); + return outcome instanceof okhttp3.Call && ((okhttp3.Call) outcome).isCanceled(); } static final class NoContentResponseBody extends ResponseBody {