[Basic] Add FileLock class #720

aciidgh · 2016-10-04T06:49:13Z

This class is a wrapper on POSIX's flock() and is part of fix towards:

rdar://problem/28448023 Prevent concurrent package workspace
manipulations

abertelrud

Looks good in general. Some comments inline.

abertelrud · 2016-10-04T07:26:17Z

Sources/Basic/Lock.swift

+    /// Create an instance of process lock with a name and the path where
+    /// the lock file can be created.
+    public init(name: String, cachePath: AbsolutePath) {
+        assert(localFileSystem.isDirectory(cachePath))


I'm not sure it's really appropriate to use assertions to check the state of file system entities. In most cases that should be a runtime error. One could argue that in this case, it's the caller's responsibility. In that case, the documentation should state that.

It's also inconsistent with the lock() function (below), which throws an error.

Agreed, an assertion based on the filesystem state is going to be prone to race conditions too, which is another argument against.

abertelrud · 2016-10-04T07:29:14Z

Sources/TestSupportExecutable/main.swift

+        try fileLockTest(Array(arguments.dropFirst()))
+    }
+} catch {
+    print(error)


This should probably get printed to stderr instead of stdout.

abertelrud · 2016-10-04T07:30:04Z

Sources/Basic/Lock.swift

+    let cachePath: AbsolutePath
+
+    /// File descriptor to the lock file.
+    private var fd: Int32 = -1


It seems that this could be an optional instead of having to use -1 (which is of course common in C because it's all that's available).

aciidgh · 2016-10-04T07:41:00Z

Thanks @abertelrud, updated PR

ddunbar · 2016-10-04T16:00:50Z

Sources/Basic/Lock.swift

+        // Save the fd to close and remove lock later.
+        fd = fileno(fp)
+        // Aquire lock on the file.
+        guard flock(fd!, LOCK_EX) == 0 else {


This should have an errno == EINTR loop.

We may want to consider implementing which has the semantics of a file lock, when exclusively used by the SwiftPM class which creates it, but which doesn't actually use flock. The typical implementation here is to atomic write the process info to a file and then point lock file (as a symbolic link) to it, if that fails, then check the contents of the file to see if that PID is alive, and then wait for that file to be removed (along with a timeout/retry loop in case that process dies).

A couple refs:

http://0pointer.de/blog/projects/locking.html

http://llvm.org/docs/doxygen/html/LockFileManager_8cpp_source.html

Note that the first ref talks about how the approach I describe isn't reliable on places where you can't effectively do an atomic FS write (an NFS scenario), so this may night be the right approach. It is however the approach LLVM is currently taking, for better or worse.

This advantage of this approach is that it is more amenable to reuse of our other APIs (for example, the work we would need to do to support timeouts, which is something we should certainly have support for).

Disclaimer: I don't have much experience with actually using file system locking. I know enough to be paranoid. :)

As discussed on slack, there is no good solution to file locking which works with NFS too so we're going to start with flock() and see how it goes. The underlying mechanism can be changed anytime and the current usecase is just a protective measure for now.

ddunbar · 2016-10-04T16:10:04Z

Sources/TestSupportExecutable/main.swift

@@ -0,0 +1,56 @@
+import Basic
+import POSIX
+import libc


Can we have a usage or documentation comment or something in this file which explains all the operations it supports? I imagine it will grow over time, and it would be nice to be able to easily survey what it can do...

I'd still like to see a "usage" style description somewhere...

... and I'd probably make the sole existing command a subcommand " lock-test 3" so it is obvious how to extend.

I forgot to comment here, I think the new parser will automatically solve this and generate usage text. We then probably only need minimal comments here, I already have a FIXME for moving to new parser once we have it

It is a subcommand right now or do you mean something else?

aciidgh · 2016-10-05T07:11:51Z

Depends on swiftlang/swift#5131

ddunbar

LGTM, I agree we can start with flock() and adapt as needed.

ddunbar · 2016-10-06T14:48:01Z

Sources/Basic/Lock.swift

+    public init(name: String, cachePath: AbsolutePath) {
+        self.name = name
+        self.cachePath = cachePath
+    }


Should deinit assert the lock has been released?

The lock is per-process so OS will release the lock once the process ends, do think assert is needed?

What if client tries to open it again?

That works (even across threads) because it will be the same process

ddunbar · 2016-10-06T14:48:50Z

Sources/TestSupportExecutable/main.swift

@@ -0,0 +1,56 @@
+import Basic
+import POSIX
+import libc


I'd still like to see a "usage" style description somewhere...

ddunbar · 2016-10-06T14:49:23Z

Sources/TestSupportExecutable/main.swift

@@ -0,0 +1,56 @@
+import Basic
+import POSIX
+import libc


... and I'd probably make the sole existing command a subcommand " lock-test 3" so it is obvious how to extend.

This class is a wrapper on POSIX's flock() and is part of fix towards: - <rdar://problem/28448023> Prevent concurrent package workspace manipulations

aciidgh · 2016-10-20T05:34:22Z

@swift-ci please test

aciidgh · 2016-10-20T06:32:12Z

@swift-ci please test OS X

aciidgh force-pushed the file-lock branch 2 times, most recently from 5714ca6 to 6cc0896 Compare October 4, 2016 06:51

aciidgh assigned ddunbar Oct 4, 2016

aciidgh force-pushed the file-lock branch from 6cc0896 to 2180334 Compare October 4, 2016 06:59

abertelrud reviewed Oct 4, 2016

View reviewed changes

aciidgh force-pushed the file-lock branch 2 times, most recently from cf5fb6f to e4950bb Compare October 4, 2016 07:40

ddunbar suggested changes Oct 4, 2016

View reviewed changes

aciidgh force-pushed the file-lock branch from e4950bb to a7d1915 Compare October 5, 2016 05:00

aciidgh mentioned this pull request Oct 5, 2016

[GlibcModuleMap] Add sys/file.h to glibc modulemap swiftlang/swift#5131

Merged

ddunbar approved these changes Oct 6, 2016

View reviewed changes

ddunbar assigned aciidgh and unassigned ddunbar Oct 6, 2016

aciidgh force-pushed the file-lock branch 2 times, most recently from 613b6a4 to b9f0292 Compare October 20, 2016 05:21

[Basic] Add FileLock class

3f537a1

This class is a wrapper on POSIX's flock() and is part of fix towards: - <rdar://problem/28448023> Prevent concurrent package workspace manipulations

aciidgh force-pushed the file-lock branch from b9f0292 to 3f537a1 Compare October 20, 2016 05:33

aciidgh merged commit bdc4a3d into swiftlang:master Oct 20, 2016

aciidgh deleted the file-lock branch October 20, 2016 06:40

[Basic] Add FileLock class #720

[Basic] Add FileLock class #720

Uh oh!

Conversation

aciidgh commented Oct 4, 2016

Uh oh!

abertelrud left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aciidgh commented Oct 4, 2016

Uh oh!

ddunbar Oct 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aciidgh commented Oct 5, 2016

Uh oh!

ddunbar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aciidgh commented Oct 20, 2016

Uh oh!

aciidgh commented Oct 20, 2016

Uh oh!

Uh oh!

ddunbar Oct 4, 2016 •

edited

Loading