[GitHub] [kvrocks] mapleFU commented on pull request #1631: Fix MSETNX not allow overriding the same key

["mapleFU (via GitHub)" <gi...@apache.org>]

mapleFU commented on PR #1631:
URL: https://github.com/apache/kvrocks/pull/1631#issuecomment-1665060859

   After a round of review I get to understand this patch. There is a problem:
   
   ```c++
   class MultiLockGuard {
    public:
     explicit MultiLockGuard(LockManager *lock_mgr, const std::vector<std::string> &keys) : lock_mgr_(lock_mgr) {
       locks_ = lock_mgr_->MultiGet(keys);
       for (const auto &iter : locks_) {
         iter->lock();
       }
     }
   
     ~MultiLockGuard() {
       // Lock with order `A B C` and unlock should be `C B A`
       for (auto iter = locks_.rbegin(); iter != locks_.rend(); ++iter) {
         (*iter)->unlock();
       }
     }
   
     MultiLockGuard(const MultiLockGuard &) = delete;
     MultiLockGuard &operator=(const MultiLockGuard &) = delete;
   
    private:
     LockManager *lock_mgr_ = nullptr;
     std::vector<std::mutex *> locks_;
   };
   ```
   
   Previously, the lock happens per-key. It was a bit wierd but would not introduce problem. Now, `exclusive` is removed, so it might introduce the deadlock problem.
   
   A proposal is that using std::lock( https://en.cppreference.com/w/cpp/thread/lock ) instead of loop in `MultiLockGuard`.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org