Skip to content

Commit

Permalink
refactor(nodebuilder): use external flock library (#3253)
Browse files Browse the repository at this point in the history
This PR is prompted by #3246. While looking through the code and understanding how `IsLocked` worked there, I realized that our fslock lib was missing a feature. Instead of asking @mastergaurang94 to fix it, I decided to completely migrate over to a more [comprehensive locking solution](https://github.com/gofrs/flock), which additionally:
* avoids the need custom code to maintain(NIHS)
* solves the problem of locking being annoying after an ungraceful shutdown
* gives us Windows and other niche OS support
	* as well as works around niche FS edge-cases
  • Loading branch information
Wondertan authored Mar 13, 2024
1 parent 3b279bc commit 648f61a
Show file tree
Hide file tree
Showing 8 changed files with 51 additions and 180 deletions.
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ require (
github.com/filecoin-project/dagstore v0.5.6
github.com/filecoin-project/go-jsonrpc v0.3.1
github.com/gammazero/workerpool v1.1.3
github.com/gofrs/flock v0.8.1
github.com/gogo/protobuf v1.3.3
github.com/golang/mock v1.6.0
github.com/gorilla/mux v1.8.1
Expand Down
48 changes: 0 additions & 48 deletions libs/fslock/lock_unix.go

This file was deleted.

49 changes: 0 additions & 49 deletions libs/fslock/locker.go

This file was deleted.

40 changes: 0 additions & 40 deletions libs/fslock/locker_test.go

This file was deleted.

29 changes: 16 additions & 13 deletions nodebuilder/config.go
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
package nodebuilder

import (
"fmt"
"io"
"os"

"github.com/BurntSushi/toml"
"github.com/gofrs/flock"
"github.com/imdario/mergo"

"github.com/celestiaorg/celestia-node/libs/fslock"
"github.com/celestiaorg/celestia-node/nodebuilder/core"
"github.com/celestiaorg/celestia-node/nodebuilder/das"
"github.com/celestiaorg/celestia-node/nodebuilder/gateway"
Expand Down Expand Up @@ -91,14 +92,15 @@ func RemoveConfig(path string) (err error) {
return
}

flock, err := fslock.Lock(lockPath(path))
flk := flock.New(lockPath(path))
ok, err := flk.TryLock()
if err != nil {
if err == fslock.ErrLocked {
err = ErrOpened
}
return
return fmt.Errorf("locking file: %w", err)
}
defer flk.Close()
if !ok {
return ErrOpened
}
defer flock.Unlock() //nolint: errcheck

return removeConfig(configPath(path))
}
Expand All @@ -117,14 +119,15 @@ func UpdateConfig(tp node.Type, path string) (err error) {
return err
}

flock, err := fslock.Lock(lockPath(path))
flk := flock.New(lockPath(path))
ok, err := flk.TryLock()
if err != nil {
if err == fslock.ErrLocked {
err = ErrOpened
}
return err
return fmt.Errorf("locking file: %w", err)
}
defer flk.Close()
if !ok {
return ErrOpened
}
defer flock.Unlock() //nolint: errcheck

newCfg := DefaultConfig(tp)

Expand Down
28 changes: 15 additions & 13 deletions nodebuilder/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@ import (
"github.com/cosmos/cosmos-sdk/crypto/hd"
"github.com/cosmos/cosmos-sdk/crypto/keyring"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/gofrs/flock"

"github.com/celestiaorg/celestia-app/app"
"github.com/celestiaorg/celestia-app/app/encoding"

"github.com/celestiaorg/celestia-node/libs/fslock"
"github.com/celestiaorg/celestia-node/libs/utils"
"github.com/celestiaorg/celestia-node/nodebuilder/node"
"github.com/celestiaorg/celestia-node/nodebuilder/state"
Expand All @@ -35,14 +35,15 @@ func Init(cfg Config, path string, tp node.Type) error {
return err
}

flock, err := fslock.Lock(lockPath(path))
flk := flock.New(lockPath(path))
ok, err := flk.TryLock()
if err != nil {
if err == fslock.ErrLocked {
return ErrOpened
}
return err
return fmt.Errorf("locking file: %w", err)
}
defer flk.Close()
if !ok {
return ErrOpened
}
defer flock.Unlock() //nolint: errcheck

ksPath := keysPath(path)
err = initDir(ksPath)
Expand Down Expand Up @@ -82,14 +83,15 @@ func Reset(path string, tp node.Type) error {
}
log.Infof("Resetting %s Node Store over '%s'", tp, path)

flock, err := fslock.Lock(lockPath(path))
flk := flock.New(lockPath(path))
ok, err := flk.TryLock()
if err != nil {
if err == fslock.ErrLocked {
return ErrOpened
}
return err
return fmt.Errorf("locking file: %w", err)
}
defer flk.Close()
if !ok {
return ErrOpened
}
defer flock.Unlock() //nolint: errcheck

err = resetDir(dataPath(path))
if err != nil {
Expand Down
7 changes: 4 additions & 3 deletions nodebuilder/init_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,13 @@ import (
"github.com/cosmos/cosmos-sdk/crypto/hd"
"github.com/cosmos/cosmos-sdk/crypto/keyring"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/gofrs/flock"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/celestiaorg/celestia-app/app"
"github.com/celestiaorg/celestia-app/app/encoding"

"github.com/celestiaorg/celestia-node/libs/fslock"
"github.com/celestiaorg/celestia-node/nodebuilder/node"
)

Expand Down Expand Up @@ -58,9 +58,10 @@ func TestIsInitForNonExistDir(t *testing.T) {

func TestInitErrForLockedDir(t *testing.T) {
dir := t.TempDir()
flock, err := fslock.Lock(lockPath(dir))
flk := flock.New(lockPath(dir))
_, err := flk.TryLock()
require.NoError(t, err)
defer flock.Unlock() //nolint:errcheck
defer flk.Close()
nodes := []node.Type{node.Light, node.Bridge}

for _, node := range nodes {
Expand Down
29 changes: 15 additions & 14 deletions nodebuilder/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,11 @@ import (

"github.com/cosmos/cosmos-sdk/crypto/keyring"
"github.com/dgraph-io/badger/v4/options"
"github.com/gofrs/flock"
"github.com/ipfs/go-datastore"
dsbadger "github.com/ipfs/go-ds-badger4"
"github.com/mitchellh/go-homedir"

"github.com/celestiaorg/celestia-node/libs/fslock"
"github.com/celestiaorg/celestia-node/libs/keystore"
"github.com/celestiaorg/celestia-node/share"
)
Expand Down Expand Up @@ -58,28 +58,29 @@ func OpenStore(path string, ring keyring.Keyring) (Store, error) {
return nil, err
}

flock, err := fslock.Lock(lockPath(path))
flk := flock.New(lockPath(path))
ok, err := flk.TryLock()
if err != nil {
if err == fslock.ErrLocked {
return nil, ErrOpened
}
return nil, err
return nil, fmt.Errorf("locking file: %w", err)
}

ok := IsInit(path)
if !ok {
flock.Unlock() //nolint:errcheck
return nil, ErrNotInited
return nil, ErrOpened
}

if !IsInit(path) {
err := errors.Join(ErrNotInited, flk.Close())
return nil, err
}

ks, err := keystore.NewFSKeystore(keysPath(path), ring)
if err != nil {
err = errors.Join(err, flk.Close())
return nil, err
}

return &fsStore{
path: path,
dirLock: flock,
dirLock: flk,
keys: ks,
}, nil
}
Expand Down Expand Up @@ -131,7 +132,7 @@ func (f *fsStore) Datastore() (datastore.Batching, error) {
}

func (f *fsStore) Close() (err error) {
err = errors.Join(err, f.dirLock.Unlock())
err = errors.Join(err, f.dirLock.Close())
f.dataMu.Lock()
if f.data != nil {
err = errors.Join(err, f.data.Close())
Expand All @@ -146,7 +147,7 @@ type fsStore struct {
dataMu sync.Mutex
data datastore.Batching
keys keystore.Keystore
dirLock *fslock.Locker // protects directory
dirLock *flock.Flock // protects directory
}

func storePath(path string) (string, error) {
Expand All @@ -158,7 +159,7 @@ func configPath(base string) string {
}

func lockPath(base string) string {
return filepath.Join(base, "lock")
return filepath.Join(base, ".lock")
}

func keysPath(base string) string {
Expand Down

0 comments on commit 648f61a

Please sign in to comment.