-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Introduce globallock as distributed locks #31908
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wolfogre
added
the
type/feature
Completely new functionality. Can only be merged if feature freeze is not active.
label
Aug 23, 2024
GiteaBot
added
the
lgtm/need 2
This PR needs two approvals by maintainers to be considered for merging.
label
Aug 23, 2024
pull-request-size
bot
added
the
size/XL
Denotes a PR that changes 500-999 lines, ignoring generated files.
label
Aug 23, 2024
github-actions
bot
added
modifies/go
Pull requests that update Go code
modifies/dependencies
labels
Aug 23, 2024
wolfogre
force-pushed
the
bugfix/globallock
branch
from
August 23, 2024 10:39
e49ee72
to
c6300df
Compare
lunny
approved these changes
Aug 23, 2024
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 23, 2024
wolfogre
commented
Aug 24, 2024
This was referenced Aug 26, 2024
silverwind
approved these changes
Aug 26, 2024
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 26, 2024
wolfogre
added
the
reviewed/wait-merge
This pull request is part of the merge queue. It will be merged soon.
label
Aug 26, 2024
GiteaBot
removed
the
reviewed/wait-merge
This pull request is part of the merge queue. It will be merged soon.
label
Aug 26, 2024
zjjhot
added a commit
to zjjhot/gitea
that referenced
this pull request
Aug 27, 2024
* giteaofficial/main: Split org Propfile README to a new tab `overview` (go-gitea#31373) [skip ci] Updated translations via Crowdin Introduce globallock as distributed locks (go-gitea#31908)
Merged
lunny
pushed a commit
that referenced
this pull request
Aug 29, 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: ```go 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. ```go 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 ```go release, err := Lock(ctx, key) if err != nil { return err } defer release() // ... ``` 2. Early release ```go release, err := Lock(ctx, key) if err != nil { return err } defer release() // ... // release the lock earlier release() // continue to do something else // ... ``` 3. Functional way ```go if err := LockAndDo(ctx, key, func(ctx context.Context) error { // ... return nil }); err != nil { return err } ```
A lot of global lock test errors #32322 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
lgtm/done
This PR has enough approvals to get merged. There are no important open reservations anymore.
modifies/dependencies
modifies/go
Pull requests that update Go code
size/XL
Denotes a PR that changes 500-999 lines, ignoring generated files.
type/feature
Completely new functionality. Can only be merged if feature freeze is not active.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
To help #31813, but do not replace it, since this PR just introduces the new module but misses some work:
#31813
has done it.#31813
has done it.So I think the most efficient way is to merge this PR first (if it's acceptable) and then finish #31813.
Design principles(Outdated, please read #31933)Use spinlock even in memory implementation
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
If we expose the mutex to callers, it's possible for callers to reuse the mutex, which causes more complexity.
For example:
That's why #31813 (comment)
In this PR, we only expose
ReleaseFunc
to callers. So callers just need to callReleaseFunc
to release the lock, and do not need to care about the lock's lifecycle.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"
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
Unlike
sync.Mutex
which will be locked forever once acquired until callingUnlock
, in the new module, 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.
If the caller don't stop what it's doing, another instance which can connect to the redis server could acquire the lock, and do the same thing, which could cause data inconsistency.
So the caller should know what happened, the solution is to return a new context which will be canceled if the lock is lost or released:
Multiple ways to use the lock