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.

Sign up for GitHub

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

Jump to bottom

New Close() method, for use when making many New() filemutexs. #2

Merged
merged 1 commit into from
Oct 28, 2017
Merged

New Close() method, for use when making many New() filemutexs. #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions filemutex_flock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,17 @@ const (
// FileMutex is similar to sync.RWMutex, but also synchronizes across processes.
// This implementation is based on flock syscall.
type FileMutex struct {
mu sync.RWMutex
fd int
mu sync.RWMutex
fd int
path string
}

func New(filename string) (*FileMutex, error) {
fd, err := syscall.Open(filename, syscall.O_CREAT|syscall.O_RDONLY, mkdirPerm)
if err != nil {
return nil, err
}
return &FileMutex{fd: fd}, nil
return &FileMutex{fd: fd, path: filename}, nil
}

func (m *FileMutex) Lock() {
Expand Down Expand Up @@ -57,3 +58,15 @@ func (m *FileMutex) RUnlock() {
}
m.mu.RUnlock()
}

// Close does an Unlock() combined with closing and unlinking the associated
// lock file. You should create a New() FileMutex for every Lock() attempt if
// using Close().
func (m *FileMutex) Close() {
if err := syscall.Flock(m.fd, syscall.LOCK_UN); err != nil {
panic(err)
Copy link

Choose a reason for hiding this comment

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

Please return err instead of panic (other panics have been removed and method signatures have been updated to return err instead)

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

}
syscall.Close(m.fd)
syscall.Unlink(m.path)
Copy link
Owner

Choose a reason for hiding this comment

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

I'm a bit worried about a function called "Close" also deleting the file. If I were using this API, I'd expect Close() to have similar semantics to os.File.Close, which closes the file descriptor but does not actually delete the file. How about having Close() simply close the file descriptor, and then adding a new function CloseAndRemove() to additionally unlink the path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there's no reason why anyone would want the lock file to exist if there will no longer be a lock held on it, so I don't think it makes sense to have 2 methods. So I could either rename Close() to CloseAndRemove(), or come up with some other shorter name that conveys things better. Finish()? This makes it clearer you're not supposed to use it again.

Copy link
Owner

Choose a reason for hiding this comment

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

The case I'm thinking of is where there are multiple processes that grab and release locks on the same file (which is one of the use cases this library was designed for)

Copy link
Contributor Author

@sb10 sb10 Aug 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, the idea is that each process does New() -> Lock() -> Close(), and only one of the processes will hold the lock at any one time. This works (I think, not strictly tested) if Close() unlinks. I don't see the use case of needing the lock file to remain on disk once Close()d.

Copy link
Contributor

@rosenhouse rosenhouse Oct 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@sb10 As an API user, I would be very surprised to find that Closeing a file handle would delete the file.

In our use case for CNI plugins, we are opening the lock on a directory containing data that needs to outlive multiple cycles of Lock and Close.

We would not be able to use this library if Close caused our data directory to be deleted.

m.mu.Unlock()
Copy link
Owner

Choose a reason for hiding this comment

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

hmm what if you call Close() after Unlock()? The go docs say of sync.Mutex:

It is a run-time error if m is not locked on entry to Unlock

It seems to me that the mu member is not even needed - perhaps just remove it entirely in this pr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you call Unlock() or Lock() after Close(), you get a panic due to bad file descriptor. The documentation for Close() suggests you don't do that. The alternative of handling the closed state in the other methods doesn't seem worthwhile since you need to know you're using it wrong regardless.

If I don't unlock at the end of Close(), however, and another Lock() is attempted, you'd get deadlock, which is much more difficult to debug than getting a panic.

Leave as is? Make the documentation more explicit? "Do not call other methods after calling Close()."?

Copy link
Contributor Author

@sb10 sb10 Aug 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

As for preventing Unlock() before Close()... again, I can only think the solution is explicit documentation, and allow the panic to happen. If sync.Mutex Unlock() -> Unlock() panics, so should FileMutex.Unlock() -> Close(); they're the same mistake by the user.

Copy link
Owner

Choose a reason for hiding this comment

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

The case I'm thinking of is where I have a FileMutex that I repeatedly Lock and Unlock, and then at the end of all this I want to Close it. This means the mutex would be in the unlocked state when I want to close it, and it would feel weird to be required to lock it before closing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the user has to be aware of the last time they wish to release the lock. The last time they call Close() instead of Unlock().

I did consider the possibility of Close() not doing what Unlock() does, and so strictly correct usage would be that you'd always have to call Unlock() before Close(), but that wasn't nice from my use case of only ever locking once and then wanting to close in a simple defer.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @alexflint that we should just drop the mu member. The existing file locking semantics are strictly stronger than those of mu. It would then simplify these things a lot.

}
124 changes: 121 additions & 3 deletions filemutex_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
package filemutex

import (
"fmt"
"io/ioutil"
"os"
"path/filepath"
"testing"
"time"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
Expand All @@ -31,14 +31,26 @@ func TestRLockUnlock(t *testing.T) {

path := filepath.Join(dir, "x")
m, err := New(path)
fmt.Println(path)
require.NoError(t, err)

m.RLock()
m.RUnlock()
}

func TestSimultaneousLock(t *testing.T) {
func TestClose(t *testing.T) {
dir, err := ioutil.TempDir("", "")
require.NoError(t, err)
defer os.RemoveAll(dir)

path := filepath.Join(dir, "x")
m, err := New(path)
require.NoError(t, err)

m.Lock()
m.Close()
}

func TestSequentialLock(t *testing.T) {
dir, err := ioutil.TempDir("", "")
require.NoError(t, err)
defer os.RemoveAll(dir)
Expand Down Expand Up @@ -72,3 +84,109 @@ func TestSimultaneousLock(t *testing.T) {
<-ch
assert.Equal(t, "released", state)
}

func TestSimultaneousLock(t *testing.T) {
dir, err := ioutil.TempDir("", "")
require.NoError(t, err)
defer os.RemoveAll(dir)

path := filepath.Join(dir, "x")
m, err := New(path)
require.NoError(t, err)

m2, err := New(path)
require.NoError(t, err)

_, err = os.Stat(path)
require.NoError(t, err)

m.Lock()

state := "waiting"
ch := make(chan struct{})
go func() {
go func() {
<-time.After(50 * time.Millisecond)
state = "waiting on lock" // for m to unlock before m2 can lock
ch <- struct{}{}
}()
m2.Lock()
state = "acquired"
ch <- struct{}{}

<-ch
m2.Unlock()
state = "closed"
ch <- struct{}{}
}()

assert.Equal(t, "waiting", state)

<-ch
assert.Equal(t, "waiting on lock", state)

m.Unlock()

<-ch
assert.Equal(t, "acquired", state)
ch <- struct{}{}

<-ch
assert.Equal(t, "closed", state)

_, err = os.Stat(path)
assert.Nil(t, err)
}

func TestSimultaneousLockWithClose(t *testing.T) {
dir, err := ioutil.TempDir("", "")
require.NoError(t, err)
defer os.RemoveAll(dir)

path := filepath.Join(dir, "x")
m, err := New(path)
require.NoError(t, err)

m2, err := New(path)
require.NoError(t, err)

_, err = os.Stat(path)
require.NoError(t, err)

m.Lock()

state := "waiting"
ch := make(chan struct{})
go func() {
go func() {
<-time.After(50 * time.Millisecond)
state = "waiting on lock" // for m to unlock before m2 can lock
ch <- struct{}{}
}()
m2.Lock()
state = "acquired"
ch <- struct{}{}

<-ch
m2.Close()
state = "closed"
ch <- struct{}{}
}()

assert.Equal(t, "waiting", state)

<-ch
assert.Equal(t, "waiting on lock", state)

m.Close()

<-ch
assert.Equal(t, "acquired", state)
ch <- struct{}{}

<-ch
assert.Equal(t, "closed", state)

_, err = os.Stat(path)
assert.NotNil(t, err)
}
20 changes: 17 additions & 3 deletions filemutex_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,9 @@ func unlockFileEx(h syscall.Handle, reserved, locklow, lockhigh uint32, ol *sysc
// FileMutex is similar to sync.RWMutex, but also synchronizes across processes.
// This implementation is based on flock syscall.
type FileMutex struct {
mu sync.RWMutex
fd syscall.Handle
mu sync.RWMutex
fd syscall.Handle
path string
}

func New(filename string) (*FileMutex, error) {
Expand All @@ -57,7 +58,7 @@ func New(filename string) (*FileMutex, error) {
if err != nil {
return nil, err
}
return &FileMutex{fd: fd}, nil
return &FileMutex{fd: fd, path: filename}, nil
}

func (m *FileMutex) Lock() {
Expand Down Expand Up @@ -91,3 +92,16 @@ func (m *FileMutex) RUnlock() {
}
m.mu.RUnlock()
}

// Close does an Unlock() combined with closing and unlinking the associated
// lock file. You should create a New() FileMutex for every Lock() attempt if
// using Close().
func (m *FileMutex) Close() {
var ol syscall.Overlapped
if err := unlockFileEx(m.fd, 0, 1, 0, &ol); err != nil {
panic(err)
}
syscall.Close(m.fd)
syscall.Unlink(m.path)
m.mu.Unlock()
}