Fix/issue 684

[Phoenix500526]

Please briefly answer these questions:
Close the issue #664 and #684

• what problem are you trying to solve? (or if there's no problem, what's the motivation for this change?)
  Close the issue [Bug]: The xlinectl won't renew the lease of the lock key #664 and [Refactor]: Add a session structure to renew lock lease automatically  #684

• what changes does this pull request make?
  Implement a session structure to auto renew the lock lease.
  Implement an Xutex, which means Xline Mutex, to describe a lock instance
  Provide an RAII implementation XutexGuard for Xutex
  Remove the LockRequest and UnlockRequest in xline-client
  Remove some unless test cases, like lock_should_timeout_when_ttl_is_set. Actually, whether the ttl is set or not, the lock in etcd won't timeout. The ttl of a lock is only used to liveness checking. FYI: what is used for about ttl in session? etcd-io/etcd#6736
  Remove the validation_lock_client.rs

• are there any non-obvious implications of these changes? (does it break compatibility with previous versions, etc)
  no

[mergify]

@Phoenix500526 Convert your pr to draft since CI failed

[mergify]

@Phoenix500526 You've modified the workflows. Please don't forget to update the .mergify.yml.

[codecov]

Codecov Report

Attention: Patch coverage is 70.04831% with 62 lines in your changes missing coverage. Please review.

Project coverage is 75.62%. Comparing base (e35b35a) to head (32fa940).
Report is 123 commits behind head on master.

Files	Patch %	Lines
crates/xline-client/src/clients/lock.rs	73.97%	38 Missing and 13 partials ⚠️
crates/xlinectl/src/command/lock.rs	0.00%	6 Missing ⚠️
crates/xlinectl/src/command/lease/keep_alive.rs	0.00%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #820      +/-   ##
==========================================
+ Coverage   75.55%   75.62%   +0.07%     
==========================================
  Files         180      185       +5     
  Lines       26938    27761     +823     
  Branches    26938    27761     +823     
==========================================
+ Hits        20353    20995     +642     
- Misses       5366     5469     +103     
- Partials     1219     1297      +78     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[bsbds]

Should we notify the user if the keep alive task exit before unlock?

[Phoenix500526]

It seems we can do nothing but panic. Do you have any better solutions?

[bsbds]

On second thought, I think it's impossible to make sure that the lock is always held during crital section, the user must assume the lock is held.

[Phoenix500526]

Shall we still need to handle this problem if it's impossible to make sure that the lock is always held during critical section?

[bsbds]

I think we could simply print an error message when the task exits.

[bsbds]

We may consider implementing this #820 (comment)

[Phoenix500526]

Currently, a user will get an error when he invokes the lock method on a Xutex of which keep-alive task exists.

[bsbds]

I suggest to use the following API to avoid confusion about how to use the lock:

    let xutex = Xutex::new(..).await?;
    xutex.put(..).await?;
    xutex.range(..).await?;
    xutex.delete_range(..).await?;
    xutex.txn(..).await?;

    let guard = xutex.lock_unsafe().await?;
    // perform some unsafe operation that may break consistency
    // ...
    drop(guard);

We could directly implement safe operations put/range/delete_range/txn for the type Xutex, and also implement a lock_unsafe method to make it compatible with the behavior of the etcd client.

[Phoenix500526]

What does ‘compatible with the behavior of the etcd client’ mean? Just renaming lock as lock_unsafe? In addition, I don't get the point why implementing put/range/delete_range is less confusing than not implementing them? The Xutex is a lock wrapper, not a KvClient wrapper. Therefore, if a user needs to do something while holding a lock, he should use a KvClient to initiate a request instead of using Xutex. Considering that, I think providing txn_with_locked_key is enough. What it does is only to generate a txn request which compares the locked key exist or not.

[mergify]

@Phoenix500526 Convert your pr to draft since CI failed

[mergify]

@Phoenix500526 Your PR is in conflict and cannot be merged.

[mergify]

@Phoenix500526 Your PR is in conflict and cannot be merged.

[rogercloud]

Mutex and its guard may be not the best way to abstract. If there's any error happens, such as lease renew failure, lock free is a good point to report error. In the mutex guard abstraction, lock free is in the drop function, which is not possible to handle for the caller.

[rogercloud]

Usually mutex guards have a lifetime bound, make sure the mutex itself lives longer than the guards. Shall we follow it?

[Phoenix500526]

The async_dropper requires the Default trait. If we introduce the lifetime bound for XutexGuard, the async_dropper may not work properly.

[bsbds]

How about we allow the user to do a manual unlock, and the unlock could return some information to the user.
For example:

{
    enum UnlockResult {
        Ok,
        // The lock may has been unlocked by the server
        Unsafe(Err),
    }
    let guard = xutex.lock_unsafe().await?;
    let result: UnlockResult = guard.unlock().await?;
    match result {
        // handles the lock result
        // ...
    }
}

[Phoenix500526]

Manually unlocking may not be a good practice since the likelihood of a user forgetting to unlock is greater than the likelihood of a panic occurring.

[Phoenix500526]

Mutex and its guard may be not the best way to abstract. If there's any error happens, such as lease renew failure, lock free is a good point to report error. In the mutex guard abstraction, lock free is in the drop function, which is not possible to handle for the caller.

But if you don't provide an RAII implementation for the lock, users may be bothered by the fact that they may forget to free a lock.

[rogercloud]

Mutex and its guard may be not the best way to abstract. If there's any error happens, such as lease renew failure, lock free is a good point to report error. In the mutex guard abstraction, lock free is in the drop function, which is not possible to handle for the caller.

But if you don't provide an RAII implementation for the lock, users may be bothered by the fact that they may forget to free a lock.

No, they won't if you put operations dealing with the locked data in a closure and release the lock while leaving the closure. For example providing the following code:

let return_value = a_xutex.map_lock( |xutex_guard| {
/// dealing with the guard
});

The return_value can tell if there's any lock related issue during the closure.

Additionally if you follow this way, the life time bound of the guard is not necessary as there's only one way to get the guard and drop it.

[mergify]

@Phoenix500526 Convert your pr to draft since CI failed

[mergify]

@Phoenix500526 Your PR is in conflict and cannot be merged.

[mergify]

@Phoenix500526 Convert your pr to draft since CI failed

[bsbds]

We could guarantee the lock safety by coupling the lock key to every update send to Xline, and the Xline server must verify the validity of the key. Please refer to https://jepsen.io/analyses/etcd-3.4.3

On the client side, we could associate KV operation methods to the lock guard to prevent user from using the lock for other purpose.

[Phoenix500526]

@Mergifyio rebase

[mergify]

rebase

✅ Branch has been successfully rebased

[CrystalAnalyst]

LGTM.