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

remove custom trylock in favor of stock sync.Mutex #1886

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chappjc
Copy link
Member

@chappjc chappjc commented Dec 15, 2021

Go 1.18 supplements Mutex with a TryLock method. For the on-demand concurrent client accesses to the cache-guarded DB, use this directly instead of our dumb homebrew trylock mutex when Go 1.18 is in use.

IMPORTANT NOTES:

  • This change does NOT adopt a new "try lock" pattern. This involves legacy code that had already established this with custom mutex type that has the same new method provided by the sync.Mutex in Go 1.18, so this just uses it.
  • While I am aware that proper uses of TryLock are few and far between, the synchronization need here involves controlling access to two data sources, a cache and a DB, where a simple CAS operation is insufficient. In particular, if there is no cached data or if it is stale (by height), a DB query must be launched on-demand, but there must only be one querying goroutine. That part is solvable with a CAS. However, subsequent callers concurrent with the DB updating goroutine (who failed to get the lock or swap) may return stale data, but if there was no cached data they must wait for the DB updater goroutine. While other solutions are readily conceivable for this, the pattern if !TryLock() { if haveCachedData { /* return stale data */ } else { Lock() /* wait */ } } solves this quite handily. This has been functioning well in production for quite some time.

The use case in pseudo code:

// First try the cache for fresh data.
cachedData, stale := checkCache(height)
if !stale {
	return cachedData
}

// Stale data.  Attempt to gain updater status.
if mtx.TryLock() {
	// Query the DB, update cache, and return fresh data.
	...
	return freshData
}
// Another goroutine is running a db query to get the updated data.

if cachedData != nil { // return stale data from cache if we have any
	return cachedData
}

// No stale data available, wait on the DB updater with a full `Lock`
mtx.Lock() // waiting, could also RLock
cachedData, _ := checkCache(height)
return cachedData

An alternative approach that does not involve locking to wait on the data, but rather a channel to receive from, used elsewhere in the project:

// TryLock will attempt to obtain an exclusive lock and a function to release
// the lock. If the lock is already held, the channel returned by TryLock will
// be closed when/if the holder of the lock calls the done function.
//
// Trylock returns a bool, busy, indicating if another caller has already
// obtained the lock. When busy is false, the caller has obtained the exclusive
// lock, and the returned func(), done, should be called when ready to release
// the lock. When busy is true, the returned channel, wait, should be received
// from to block until the updater has released the lock.
func (cl *CacheLock) TryLock(addr string) (busy bool, wait chan struct{}, done func()) {

Also update CI to use go 1.18-beta1.

@chappjc chappjc changed the title trylock: Use sync.Mutex in go1.18+, update CI for beta1 trylock: use sync.Mutex in go1.18+ Nov 1, 2022
@chappjc chappjc added this to the 6.2 milestone Nov 2, 2022
@chappjc chappjc changed the title trylock: use sync.Mutex in go1.18+ remove custom trylock in favor of stock sync.Mutex Dec 8, 2022
Go 1.18 supplements Mutex with a TryLock method.
Use this directly instead of our dumb homebrew
trylock mutex when Go 1.18 is in use.
@chappjc chappjc marked this pull request as ready for review December 8, 2022 22:14
Copy link
Collaborator

@ukane-philemon ukane-philemon left a comment

Choose a reason for hiding this comment

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

Looks straight-forward.

@@ -24,7 +24,7 @@ jobs:
working-directory: ./cmd/dcrdata

- name: Install Linters
run: "curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.48.0"
run: "curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v1.50.1"
Copy link
Collaborator

Choose a reason for hiding this comment

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

current version master is v1.55.2, consider updating to v1.58.0

@@ -1,6 +1,9 @@
// Copyright (c) 2021, The Decred developers
// Copyright (c) 2021-2022, The Decred developers
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can update this the next time you commit.

@@ -0,0 +1,13 @@
// Copyright (c) 2022, The Decred developers
Copy link
Collaborator

Choose a reason for hiding this comment

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

This too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants