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

Fix build on Plan 9 #17

Merged
merged 1 commit into from
Jul 26, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 1 addition & 5 deletions fslock.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"os"
"path/filepath"
"strings"
"syscall"

util "github.com/ipfs/go-ipfs-util"
logging "github.com/ipfs/go-log"
Expand All @@ -30,10 +29,7 @@ func Lock(confdir, lockFileName string) (io.Closer, error) {
lk, err := lock.Lock(lockFilePath)
if err != nil {
switch {
case err == syscall.EAGAIN:
// EAGAIN == someone else has the lock
fallthrough
case strings.Contains(err.Error(), "resource temporarily unavailable"):
case lockedByOthers(err):
return lk, &os.PathError{
Op: "lock",
Path: lockFilePath,
Expand Down
33 changes: 33 additions & 0 deletions fslock_plan9.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package fslock

import "strings"

// Opening an exclusive-use file returns an error.
// The expected error strings are:
//
// - "open/create -- file is locked" (cwfs, kfs)
// - "exclusive lock" (fossil)
// - "exclusive use file already open" (ramfs)
//
// See https://github.com/golang/go/blob/go1.15rc1/src/cmd/go/internal/lockedfile/lockedfile_plan9.go#L16
var lockedErrStrings = [...]string{
Copy link
Contributor

Choose a reason for hiding this comment

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

Agree that the structure you've added here seems like a reasonable way to extend the lock function to plan9, and this follows the definitions in the standard library, so seems fine.

The one semantic thing I'm not clear on that is worth checking if you haven't yet, is that these errors will cover both the "someone else has the lock" error that they will trigger the overall Lock method returning, and the lock is already held by us case in the main lock switch statement.

I haven't looked to see if any of the callers of Lock do different things based on those errors, and my quick read at how plan9 is structuring its errors doesn't leave me overly optimistic that we'd be able to efficiently learn if the existing exclusive fid is in the same process, but it's probably worth checking to make sure there isn't code that's using those semantics.

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 the lock is held by us, we get this error:

file "/tmp/filelock/my-test.lock" already locked

If the lock is held by another process, we get this error:

cannot acquire lock: Lock Create of /tmp/filelock/my-test.lock failed: open /tmp/filelock/my-test.lock: '/tmp/filelock/my-test.lock' file is locked

I think I already distinguish between these two errors by checking if the error string contains "Lock Create of" (similar to isLockCreatePermFail function). We depends on go4.org/lock returning two kinds of errors, but I think it's good enough solution.

"file is locked",
"exclusive lock",
"exclusive use file already open",
}

// isLockedPlan9 return whether an os.OpenFile error indicates that
// a file with the ModeExclusive bit set is already open.
func isLockedPlan9(s string) bool {
for _, frag := range lockedErrStrings {
if strings.Contains(s, frag) {
return true
}
}
return false
}

func lockedByOthers(err error) bool {
s := err.Error()
return strings.Contains(s, "Lock Create of") && isLockedPlan9(s)
}
12 changes: 12 additions & 0 deletions fslock_posix.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// +build !plan9

package fslock

import (
"strings"
"syscall"
)

func lockedByOthers(err error) bool {
return err == syscall.EAGAIN || strings.Contains(err.Error(), "resource temporarily unavailable")
}
65 changes: 65 additions & 0 deletions fslock_test.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
package fslock_test

import (
"bufio"
"io/ioutil"
"os"
"os/exec"
"path"
"strings"
"testing"
"time"

lock "github.com/ipfs/go-fs-lock"
)
Expand Down Expand Up @@ -94,3 +99,63 @@ func TestLockMultiple(t *testing.T) {
assertLock(t, confdir, lockFile1, false)
assertLock(t, confdir, lockFile2, false)
}

func TestLockedByOthers(t *testing.T) {
const (
lockedMsg = "locked\n"
lockFile = "my-test.lock"
wantErr = "someone else has the lock"
)

if os.Getenv("GO_WANT_HELPER_PROCESS") == "1" { // child process
confdir := os.Args[3]
if _, err := lock.Lock(confdir, lockFile); err != nil {
t.Fatalf("child lock: %v", err)
}
os.Stdout.WriteString(lockedMsg)
time.Sleep(10 * time.Minute)
return
}

confdir, err := ioutil.TempDir("", "go-fs-lock-test")
if err != nil {
t.Fatalf("creating temporary directory: %v", err)
}
defer os.RemoveAll(confdir)

// Execute a child process that locks the file.
cmd := exec.Command(os.Args[0], "-test.run=TestLockedByOthers", "--", confdir)
cmd.Env = append(os.Environ(), "GO_WANT_HELPER_PROCESS=1")
cmd.Stderr = os.Stderr
stdout, err := cmd.StdoutPipe()
if err != nil {
t.Fatalf("cmd.StdoutPipe: %v", err)
}
if err = cmd.Start(); err != nil {
t.Fatalf("cmd.Start: %v", err)
}
defer cmd.Process.Kill()

// Wait for the child to lock the file.
b := bufio.NewReader(stdout)
line, err := b.ReadString('\n')
if err != nil {
t.Fatalf("read from child: %v", err)
}
if got, want := line, lockedMsg; got != want {
t.Fatalf("got %q from child; want %q", got, want)
}

// Parent should not be able to lock the file.
_, err = lock.Lock(confdir, lockFile)
if err == nil {
t.Fatalf("parent successfully acquired the lock")
}
pe, ok := err.(*os.PathError)
if !ok {
t.Fatalf("wrong error type %T", err)
}
if got := pe.Error(); !strings.Contains(got, wantErr) {
t.Fatalf("error %q does not contain %q", got, wantErr)
}
}