Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor globallock #31933

Merged
merged 2 commits into from
Aug 29, 2024
Merged

Refactor globallock #31933

merged 2 commits into from
Aug 29, 2024

Conversation

wolfogre
Copy link
Member

@wolfogre wolfogre commented Aug 28, 2024

Follow #31908. The main refactor is that it has removed the returned context of Lock.

The returned context of Lock in old code is to provide a way to let callers know that they have lost the lock. But in most cases, callers shouldn't cancel what they are doing even it has lost the lock. And the design would confuse developers and make them use it incorrectly.

See the discussion history: #31813 (comment) and #31813 (comment)

It's a breaking change, but since the new module hasn't been used yet, I think it's OK to not add the pr/breaking label.

Design principles

It's almost copied from #31908, but with some changes.

Use spinlock even in memory implementation (unchanged)

In actual use cases, users may cancel requests. sync.Mutex will block the goroutine until the lock is acquired even if the request is canceled. And the spinlock is more suitable for this scenario since it's possible to give up the lock acquisition.

Although the spinlock consumes more CPU resources, I think it's acceptable in most cases.

Do not expose the mutex to callers (unchanged)

If we expose the mutex to callers, it's possible for callers to reuse the mutex, which causes more complexity.

For example:

lock := GetLocker(key)
lock.Lock()
// ...
// even if the lock is unlocked, we cannot GC the lock,
// since the caller may still use it again.
lock.Unlock()
lock.Lock()
// ...
lock.Unlock()

// callers have to GC the lock manually.
RemoveLocker(key)

That's why #31813 (comment)

In this PR, we only expose ReleaseFunc to callers. So callers just need to call ReleaseFunc to release the lock, and do not need to care about the lock's lifecycle.

release, err := locker.Lock(ctx, key)
if err != nil {
    return err
}
// ...
release()

// if callers want to lock again, they have to re-acquire the lock.
release, err := locker.Lock(ctx, key)
// ...

In this way, it's also much easier for redis implementation to extend the mutex automatically, so that callers do not need to care about the lock's lifecycle. See also #31813 (comment)

Use "release" instead of "unlock" (unchanged)

For "unlock", it has the meaning of "unlock an acquired lock". So it's not acceptable to call "unlock" when failed to acquire the lock, or call "unlock" multiple times. It causes more complexity for callers to decide whether to call "unlock" or not.

So we use "release" instead of "unlock" to make it clear. Whether the lock is acquired or not, callers can always call "release", and it's also safe to call "release" multiple times.

But the code DO NOT expect callers to not call "release" after acquiring the lock. If callers forget to call "release", it will cause resource leak. That's why it's always safe to call "release" without extra checks: to avoid callers to forget to call it.

Acquired locks could be lost, but the callers shouldn't stop

Unlike sync.Mutex which will be locked forever once acquired until calling Unlock, for distributed lock, the acquired lock could be lost.

For example, the caller has acquired the lock, and it holds the lock for a long time since auto-extending is working for redis. However, it lost the connection to the redis server, and it's impossible to extend the lock anymore.

In #31908, it will cancel the context to make the operation stop, but it's not safe. Many operations are not revert-able. If they have been interrupted, then the instance goes corrupted. So Lock won't return ctx anymore in this PR.

Multiple ways to use the lock

  1. Regular way
release, err := Lock(ctx, key)
if err != nil {
    return err
}
defer release()
// ...
  1. Early release
release, err := Lock(ctx, key)
if err != nil {
    return err
}
defer release()
// ...
// release the lock earlier
release()
// continue to do something else
// ...
  1. Functional way
if err := LockAndDo(ctx, key, func(ctx context.Context) error {
    // ...
    return nil
}); err != nil {
    return err
}

@wolfogre wolfogre added the type/enhancement An improvement of existing functionality label Aug 28, 2024
@wolfogre wolfogre added this to the 1.23.0 milestone Aug 28, 2024
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 28, 2024
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 28, 2024
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Aug 28, 2024
@wolfogre wolfogre requested review from lunny and wxiaoguang August 28, 2024 07:58
Copy link
Member Author

@wolfogre wolfogre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can add a new method in the future, like

LockAndWatch(ctx context.Context, key string) (context.Context, ReleaseFunc, error)

For some operations which are revertable and care about lock losing, if they really exist.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Aug 28, 2024
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Aug 28, 2024
@wolfogre wolfogre added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Aug 29, 2024
@lunny lunny enabled auto-merge (squash) August 29, 2024 03:27
@lunny lunny merged commit bc0977f into go-gitea:main Aug 29, 2024
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Aug 29, 2024
zjjhot added a commit to zjjhot/gitea that referenced this pull request Aug 30, 2024
* giteaofficial/main:
  [skip ci] Updated translations via Crowdin
  Update JS and PY dependencies (go-gitea#31940)
  Fix search team (go-gitea#31923)
  Upgrade micromatch to 4.0.8 (go-gitea#31939)
  Refactor globallock (go-gitea#31933)
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Nov 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/go Pull requests that update Go code size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants