nightlyone/lockfile

Use symlinks instead of hardlinks on unix platforms

insomniacslk opened this issue · 7 comments

Hello,
first off, thanks for your library! While working on a project that uses it, I found (u-root/u-root#954) that lockfile uses hard links, and this fails if the source and the destination files are on different mount points. Hard links are not really necessary on UNIX platforms for locking purposes. If that sounds OK I will send a PR to use symlinks if the platform is a UNIX system. Thanks!

cc @hugelgupf @rminnich

Hello @insomniacslk ,

thanks for the interest in this library. The use case I created this library for is creating lock files in a shared tmpfs to signal whether a process is running or not in order to ensure it runs only once.

The example at https://godoc.org/github.com/nightlyone/lockfile#example-Lockfile shows that you need to use it in the systemwide location for temporary files to ensure correct operation.

I looked at the usage in u-root and it seems to have a different use case and is also using it differently.
So once this bug in u-root is fixed, then everything works beautifully and as expected 😏

Could you try it and confirm so I can close the bug?

Not everyone running u-root will have the same systemwide tmpfs storage available.

Otoh, why hardlinks and not symlinks? Even in tmpfs, symlinks are just as atomically created as hard links. os.SameFile would have to be replaced with something calling Readlink. But that seems to be about it?

@hugelgupf systemwide tmpfs storage is not a requirement for u-boot, but suggested for correct usage of this locking library. Any other shared storage directory with atomic hardlink, remove and rename works just as well for this library.

I don't know yet what are the file system requirements of u-boot. Maybe they differ too much. So prepared a small list of questions to clear up my picture of this issue before jumping to a solution:

  • What error you see, when you use the library as in the example code?
  • What file systems are involved on which operating systems?
  • Why does the code in the example doesn't work for you?
  • Could you describe what error you see?
  • Could you provide additional details of the analysis here so we can work out a solution?

If your use case turns out to be too different from the problem this library tries to solve, please replace it. It could be that something that uses mandatory locking is a better fit for your purpose.

If your goal is to run it once per directory on different machines involving shared file systems, this locking library simply won't support that use case ever, since it is out of scope. I deeply apologize if that is not clear enough from the documentation provided and would love some hints where you would have expected that information and in which form.

In this case, EXDEV is the error code, as the library is trying to create cross-filesystem links. This would happen e.g. if you used this library on an overlay file system. Symlinks are cross-filesystem compatible and would serve the same function in this library without any reduced functionality.

I'm not sure why you wouldn't consider using symlinks instead of hard links, as it would solve your and our use case, but I might as well write my own similar library.

@hugelgupf Sorry for not expressing the hardlink requirement more clearly in the code.

The locking protocol creates a temporary file, writing stuff in it and then gives that stuff a new name, deletes the temporary file and expects the content to survive.

I really have no idea how that locking protocol should work with a symlink.

With a symlink when you delete the file you symlinked, you get a stale symlink and loose all the content.
With a hardlink it doesn't matter which one of the two you delete, the contents stay.

The library looks at that content to detect dead owners. So I see no way a symlink could work here.

To fix your problem at hand, I would suggest fixing the library usage to be like:

diff --git a/pkg/uroot/builder/bb/bb.go b/pkg/uroot/builder/bb/bb.go
index 90cc783e..ddba00bf 100644
--- a/pkg/uroot/builder/bb/bb.go
+++ b/pkg/uroot/builder/bb/bb.go
@@ -15,6 +15,7 @@ import (
        "go/token"
        "go/types"
        "io/ioutil"
+       "net/url"
        "os"
        "path"
        "path/filepath"
@@ -74,6 +75,7 @@ func BuildBusybox(env golang.Environ, pkgs []string, binaryPath string) error {
        }
 
        bblock := filepath.Join(urootPkg.Dir, "bblock")
+       bblock := filepath.Join(os.TempDir(), "u-root-bb_"+url.PathEscape(bblock))
        // Only one busybox can be compiled at a time.
        //
        // Since busybox files all get rewritten in

os.TempDir is no more than a convention.

I guess I'm unclear on why you need to delete the pointed-to file at creation time. You could keep the file as well as the symlink, and delete it once it's detected to be stale.

The existence of the symlink would be the lock (and is atomic), and you can follow the symlink to find out if it's stale, which is a racy operation in either case.

frozen due to age.