Remove some uses of synchronized
#3024
Conversation
Motivation: With java-21 and v-threads synchronization can cause deadlocks because carrier threads cannot release a v-thread that is in a synchronized block. Modifications: Identify the cases where we're using synchronization and move them to the equivalent v-thread safe ReentrantLock. Result: Better v-thread safety.
| @@ -67,7 +68,7 @@ final class GradientCapacityLimiter implements CapacityLimiter { | |||
|
|
|||
| private static final Logger LOGGER = LoggerFactory.getLogger(GradientCapacityLimiter.class); | |||
|
|
|||
| private final Object lock = new Object(); | |||
| private final ReentrantLock lock = new ReentrantLock(); | |||
There was a problem hiding this comment.
AimdCapacityLimiter uses stateUpdater.compareAndSet(this, UNLOCKED, LOCKED). While I don't have any personal preference towards one of the approaches (not sure if it's worth a banchmark), it would be great to make both implementations consistent.
There was a problem hiding this comment.
I'd prefer not to introduce behavior changes in this PR and moving from a blocking lock to a spin-lock feels non-trivial.
Tangentially, I suspect spin-locks are not going to play well with v-threads either since that will likely cause a v-thread to not be able to be de-schedule since it's not 'blocked' by anything, resulting in a similar deadlock situation.
There was a problem hiding this comment.
agreed on lets keep the same synchronization approach in this PR, and can consider changes/improvements in approach as followup.
There was a problem hiding this comment.
I'm fine with the follow-up. With the above argument about v-threads, seems reasonable to use ReentrantLock in Aimd as well. IIRC our discussion with @tkountis, there were no other motivations for that spin-lock except just an attempt to avoid synchronized.
| @@ -67,7 +68,7 @@ final class GradientCapacityLimiter implements CapacityLimiter { | |||
|
|
|||
| private static final Logger LOGGER = LoggerFactory.getLogger(GradientCapacityLimiter.class); | |||
|
|
|||
| private final Object lock = new Object(); | |||
| private final ReentrantLock lock = new ReentrantLock(); | |||
There was a problem hiding this comment.
I'm fine with the follow-up. With the above argument about v-threads, seems reasonable to use ReentrantLock in Aimd as well. IIRC our discussion with @tkountis, there were no other motivations for that spin-lock except just an attempt to avoid synchronized.
Motivation:
With java-21 and v-threads synchronization can cause deadlocks because carrier threads cannot release a v-thread that is in a synchronized block.
Modifications:
Identify the cases where we're using synchronization and move them to the equivalent v-thread safe ReentrantLock.
Result:
Better v-thread safety.