Replace synchronized blocks in OkHttpCall with atomic CAS #4656
Replace synchronized blocks in OkHttpCall with atomic CAS #4656Amir-yazdanmanesh wants to merge 4 commits intosquare:trunkfrom
Conversation
…e#4072) The testJdkN tasks copied classpath and testClassesDirs from the built-in test task, which broke the Gradle task dependency graph. Re-running a test task would not recompile main/test sources because no dependency on compilation tasks was established. Wire directly to sourceSets.test outputs instead, giving Gradle implicit task dependencies through file collection producers. This also eliminates the tasks.getByName("test") eager realization anti-pattern.
1. Imports: Added AtomicBoolean and AtomicReference; removed GuardedBy 2. Fields: Replaced rawCall + creationFailure + executed with AtomicBoolean executed and AtomicReference<Object> rawCallState (merged call/failure into one atomic ref) 3. getRawCall(): Rewrote with CAS — reads state atomically, creates call on miss, uses compareAndSet to publish. Loser of a race uses the winner's result 4. throwStateFailure(): New static helper to rethrow stored failures 5. request() / timeout(): Removed synchronized keyword (now lock-free via getRawCall()) 6. execute(): Uses executed.compareAndSet(false, true) instead of synchronized block 7. enqueue(): Same CAS for executed check; delegates to getRawCall() instead of inlining creation 8. isExecuted(): Returns executed.get() instead of synchronized read 9. cancel() / isCanceled(): Read rawCallState.get() instead of synchronized block
| The locks don't guard long-running work, only state changes. I don't think this is a big deal. Has anyone demonstrated an actual problem or are we just speculating? |
You're right, the synchronized blocks here are short and don't guard long-running work, so pinning should be brief. I don't have benchmarks demonstrating an actual The change was motivated by the general guidance to avoid synchronized with virtual threads, but I understand that without evidence of a real issue, the added complexity Happy to close this if you'd prefer to keep the current approach until a concrete problem is reported. |
| I need to review it more closely. The last attempt at this had subtle races because it used two atomic states. This one is different, but it still warrants exhaustively thinking through the state transitions in the context of concurrency. |
Happy to walk through the state transitions while you review: |
2 participants
Summary
synchronized(this)blocks inOkHttpCallwith lock-free atomic operations to avoid pinning virtual threads (Project Loom)rawCallandcreationFailurefields into a singleAtomicReference<Object> rawCallStatewith state transitionsnull → Callornull → Throwable(write-once semantics)
boolean executedwithAtomicBooleanusingcompareAndSetfor the execute-once guardCloses #4297
Motivation
synchronizedblocks pin virtual threads to platform threads, defeating the purpose of Project Loom. Since the guarded code protects single-instance creation withminimal contention, atomic CAS is a better fit than
ReentrantLock. See #4297.Design
The key insight is merging
rawCall+creationFailureinto oneAtomicReference. This avoids the race condition from #4301 where separate references could get out ofsync. Once the reference is set to a non-null value (either a
Callor aThrowable), it never changes.If two threads race to create the call, both call
createRawCall()but only one CAS wins. The loser uses the winner's result. This is safe becausecreateRawCall()isside-effect-free.
Test plan
./gradlew :retrofit:java-test:check— all tests pass across JDK 8, 10-24 (including concurrency tests in CallTest.java)