Replace synchronized in OkHttpCall with atomics - #4755
Closed
HuzaifaChaudary wants to merge 1 commit into
Closed
Conversation
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.
Collaborator
|
LLM usage is forbidden. Next occurrence results in a ban. |
Collaborator
|
Didn't we already determine this is harmless anyway? We're guarding trivial CPU work. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #4297
OkHttpCallguarded three fields with the object monitor: the lazily createdokhttp3.Call, the throwable that creating it threw, and theexecutedflag. Holding a monitor across call creation pins the carrier thread when the caller is a virtual thread, which is what the issue is about.Following the suggestion on the issue, this uses atomics rather than a
ReentrantLock.executedbecomes anAtomicBoolean, so the one-shot check inenqueueandexecuteis a singlecompareAndSetinstead of a read and a write under a lock.rawCallandcreationFailurebecome a singleAtomicReferenceholding whichever of the two happened. Exactly one of them does, and both have to be remembered so later callers see the same outcome, so one field models it better than two. Creation is no longer serialised: two threads arriving together will both create a call and one wins the swap. An unexecutedokhttp3.Callholds no connection, so discarding the loser costs nothing, which is the tradeoff you described.request(),timeout(),isExecuted(),cancel()andisCanceled()no longer take the monitor at all.No new test. The behaviour is unchanged and the existing suite already covers it, including the "Already executed." case in
CallTest, and a concurrency test for this would be timing dependent without proving much.:retrofit:java-test:testJdk21passes, 343 tests across 15 classes, and nothing outsideOkHttpCallreferenced either removed field.