Refactor OkHttpCall to use CAS instead of synchronized (#4297)#4301
Refactor OkHttpCall to use CAS instead of synchronized (#4297)#4301qwexter wants to merge 4 commits intosquare:trunkfrom
Conversation
Replace synchronized blocks with atomic compare-and-swap logic. It prevent virtual thread pinning and reduce contention. See square#4297 for more details.
qwexter
force-pushed
the
refactorOkHttpCallToUseCAS
branch
from
86cde28 to
91b4ebf
Compare
JakeWharton
left a comment
JakeWharton
left a comment
There was a problem hiding this comment.
Still think this needs some work. I would probably look to simplify things, such as merge the failure and call reference into one, and potentially even include the cancel state as new Object() tombstone reference. I need to spend more time with this code before I can move forward with it.
| rawCallRef.compareAndSet(null, newCall); | ||
| return rawCallRef.get(); |
There was a problem hiding this comment.
| rawCallRef.compareAndSet(null, newCall); | |
| return rawCallRef.get(); | |
| return rawCallRef.compareAndSet(null, newCall) | |
| ? newCall | |
| : rawCallRef.get(); |
| call = createRawCall(); | ||
| rawCallRef.compareAndSet(null, call); |
There was a problem hiding this comment.
This code is now racy. Another thread could be performing the same operation in parallel and its okhttp3.Call might "win" causing this instance to be a duplicate.
There was a problem hiding this comment.
I agree. If you have some ideas, pls share it. I'll hope I can get back to this with more resources at the end of September. If it will be needed at that time.
| if (call == null && failure == null) { | ||
| try { | ||
| call = createRawCall(); | ||
| rawCallRef.compareAndSet(null, call); |
There was a problem hiding this comment.
Circling back on this in case it's still active. As @JakeWharton noted, another thread could enter this block and call createRawCall(). Even though only one compareAndSet(...) might succeed, the other still ends up creating a duplicate instance.
Hi @qwexter, just curious if this race condition was considered acceptable here, or if there's a plan to mitigate it without reintroducing locking?
There was a problem hiding this comment.
Yes, I agree. I think I agree that the winner-takes-all approach mentioned in the discussion is too straightforward.
I didn't have any issues with my unit tests, but I might have missed something.
3 participants
Replace synchronized blocks with atomic compare-and-swap logic.
It prevent virtual thread pinning and reduce contention.
See #4297 for more details.
CHANGELOG.md's "Unreleased" section has been updated, if applicable.