[core] Make ObjectBufferPool::FreeObjects lock free#57833
[core] Make ObjectBufferPool::FreeObjects lock free#57833codope wants to merge 1 commit intoray-project:masterfrom
ObjectBufferPool::FreeObjects lock free#57833Conversation
Signed-off-by: Sagar Sumit <sagarsumit09@gmail.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request makes ObjectBufferPool::FreeObjects lock-free by removing an unnecessary absl::MutexLock. The justification provided is sound: PlasmaClient is internally synchronized, and this function does not access any other state of ObjectBufferPool that would require locking. The changes in both the implementation file (.cc) and the header file (.h) are consistent and correct. Removing the lock and the corresponding ABSL_LOCKS_EXCLUDED annotation is appropriate and should lead to a performance improvement by reducing lock contention. The change is well-contained and appears to be safe.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| void ObjectBufferPool::FreeObjects(const std::vector<ObjectID> &object_ids) { | ||
| absl::MutexLock lock(&pool_mutex_); | ||
| RAY_CHECK_OK(store_client_->Delete(object_ids)); | ||
| } |
There was a problem hiding this comment.
Bug: Concurrency Issue in Object Management
Removing the mutex lock from FreeObjects introduces a race condition. This allows FreeObjects to concurrently delete Plasma objects while other operations, like WriteChunk or AbortCreateInternal, are still active on the same object. This can lead to crashes, undefined behavior, or state corruption.
edoakes
left a comment
edoakes
left a comment
There was a problem hiding this comment.
It's not immediately obvious that this is safe because other methods in the ObjectBufferPool make multiple calls in sequence into the store_client_. Previously, these calls were guaranteed to execute transactionally with respect to this Delete call, but with this change the Delete can be interleaved.
If there's a strong motivation to remove the lock, then let's audit the other callsites closely and make sure that there are no correctness issues due to the interleaving, else I'd just leave it be for now.
|
This pull request has been automatically marked as stale because it has not had You can always ask for help on our discussion forum or Ray's public slack channel. If you'd like to keep this open, just leave any comment, and the stale label will be removed. |
github-actions
Bot
added
the
stale
|
ok closing it for now. I will go through the other callsites and revive the PR if needed. |
3 participants
Description
Make
ObjectBufferPool::FreeObjectslock free. Thepool_mutex_lock inFreeObjectsprovides no actual synchronization benefit because:See #57550 (comment) for the discussion.