Lock Improvements for [SR-12851] #139

tgymnich · 2020-10-06T20:21:58Z

This PR contains the following improvements to locks needed for SR-12851.

~~1. Added ReadWriteLock.~~ (Using GCD instead for windows compatiblity)
2. Added property wrapper @ThreadLocal to address an issue where FileLock could be used in way that would unlock/lock the lock from another thread without the user realizing. --> Making FileLock thread safe
3. Enabled FileLock to obtain an exclusive or a shared lock.

This is PR #130 reopened on the main branch.

MaxDesiatov · 2020-10-06T20:46:55Z

@swift-ci please test

MaxDesiatov · 2020-10-17T07:27:07Z

@swift-ci please test

tomerd · 2020-10-19T23:44:52Z

Sources/TSCBasic/FileSystem.swift

+        if let queue = lockFiles[path] {
+            fileQueue = queue
+        } else {
+            fileQueue = DispatchQueue(label: "foo", attributes: .concurrent)


I forgot to remove the placeholder. This should be org.swift.swiftpm.in-memory-file-system.file-queue

tomerd · 2020-10-19T23:45:39Z

Sources/TSCBasic/FileSystem.swift

+            fileQueue = DispatchQueue(label: "foo", attributes: .concurrent)
+            lockFiles[path] = fileQueue
+        }
+        lockFilesLock.unlock()


use lock.withLock?

If I would use lock.withLock I cannot leave var fileQueue: DispatchQueue unassigned. Making it optional would mean that I somehow have to handle the case where fileQueue is nil.

I guess it could be defined as var fileQueue: DispatchQueue!, or do we not use this pattern in this repo?

We generally try to avoid force unwrap, but there are exceptions if it makes the rest of the code safer and if there's really no way for the force unwrap to fail.

But in this case I think withLock on a regular Lock is also declared as public func withLock<T>(_ body: () throws -> T) rethrows -> T, so could you return the fileQueue from the withLock closure?

But in this case I think withLock on a regular Lock is also declared as public func withLock(_ body: () throws -> T) rethrows -> T, so could you return the fileQueue from the withLock closure?

that's a great idea

tomerd · 2020-10-19T23:54:19Z

Sources/TSCBasic/Lock.swift

@@ -11,6 +11,11 @@
 import Foundation
 import TSCLibc

+public enum LockType {


should this renamed to FileLockType or nested under FileLock?

I would prefer to keep the name unchanged since FileLock might not be the only shared lock in the future.

I see your point, but personally prefer to follow the rule of three with generalizations. since this is not a stable API I am not too worried about API surface area, but as a rule of thumb introducing top level types is something to do with care

Since API stability is of no concern let's put it inside of FileLock.

abertelrud

Thanks a lot for your work here!

I have a few comments about some of the more generic additions to ToolSupportCore, some of which I hope won't be needed soon because of SwiftSystem (https://swift.org/blog/swift-system/) and others that would be better as separate PRs, if they should be in ToolsSupportCore.

Would it be possible to focus this PR on just the file locking, even if it means keeping a bit more code in that class, and discussing how to factor this out as part of other PRs? Thanks!

abertelrud · 2020-10-19T23:53:00Z

Sources/TSCBasic/WeakDictionary.swift

+ */
+
+/// A dictionary that only keeps weak references to its values.
+struct WeakDictionary<Key: Hashable, Value: AnyObject> {


Since this isn't a generally usable dictionary (it doesn't have the regular Dictionary operations), would it be better for this use case to have the one usage case of WeakDictionary in this PR have a regular Dictionary with a weak reference as its value (using a private struct in the class that needs it)? My concern would be that this looks like a general-type of purpose Dictionary but can't be used as such, since it doesn't conform to the other Dictionary API.

abertelrud · 2020-10-19T23:57:30Z

Sources/TSCBasic/FileSystem.swift

@@ -502,6 +515,10 @@ public class InMemoryFileSystem: FileSystem {

    /// The root filesytem.
    private var root: Node
+    /// A map that keeps weak references to all locked files.
+    private var lockFiles = WeakDictionary<AbsolutePath, DispatchQueue>()


Since this seems to be the only use of WeakDictionary, I think it might be simpler in this case to use a regular Dictionary whose value is a weak reference (which might require a private struct in this case). This is in reference to the comment for the WeakDictionary.swift file.

+1 for proposing such generalizations as separate followup PR so can be discussed individually

abertelrud · 2020-10-20T00:03:49Z

Sources/TSCBasic/Thread.swift

+}
+
+/// `ThreadLocal` properties are  thread-specific. Every thread has its own instance of the wrapped property.
+@propertyWrapper final class ThreadLocal<Value: Defaultable> {


ThreadLocal as a property wrapper seems very useful, but feels too general for this PR. It feels more like a language-level feature, and we can discuss whether that should be in ToolsSupportCore.

For this case, with the file locking, would it be possible to use Foundation.Thread's threadDictionary directly?

+1 for proposing such generalizations as separate followup PR so can be discussed individually

I think I would be easier to leave FileLock thread-unsafe for the moment and to fix this issue once @ThreadLocal and @AutoClosing are available in SwiftSystem or ToolSupportCore.

abertelrud · 2020-10-20T00:11:58Z

Sources/TSCBasic/Thread.swift

+}
+
+/// Automatically closes the wrapped file descriptor on deinit.
+@propertyWrapper final class AutoClosing: NSObject, Defaultable {


As with the other property wrapper, I think this could have a lot of utility but think it would be better to discuss as a follow-on PR to simplify the code. I'm also hopeful that things like this will be able to come from SwiftSystem so that ToolSupportCore doesn't need to implement it (https://swift.org/blog/swift-system/).

+1 for proposing such generalizations as separate followup PR so can be discussed individually

tomerd

this is great @tgymnich. left some suggestions inline.

tomerd · 2020-10-20T01:58:23Z

also fwiw would love this see this merged as its needed for other use cases as well!

This reverts commit fcf119d.

tgymnich · 2020-10-20T12:30:24Z

@abertelrud @tomerd thank you for your feedback! I will start a discussion on the forums about adding @AutoClosing to SwiftSystem.

Update:
@AutoClosing Forum Post

abertelrud · 2020-10-20T18:14:32Z

Thank you very much for making the changes and for starting that post! To be clear, I think such abstractions are a great idea, but would love to see them available more generally for all Swift packages, and after broader discussion.

ToolsSupportCore originally came about because there was a lot of general-purpose code written for SwiftPM that didn't really belong in the package manager itself. But I'm really excited about SwiftSystem and some of the other packages (e.g. SwiftArgumentParser). Hopefully we'll be able to replace more and more of ToolsSupportCore with functionality from other more "polished" packages over time.

And to echo what Tom said, I, too, am really excited about the repository caching and am really eager to see it become part of SwiftPM! I'll take a look at the revised PR in the next few hours. Thanks!

tomerd · 2020-10-20T18:25:41Z

Sources/TSCBasic/CMakeLists.txt

@@ -48,7 +48,9 @@ add_library(TSCBasic
  TerminalController.swift
  Thread.swift
  Tuple.swift
-  misc.swift)
+  misc.swift
+  WeakDictionary.swift)


still in use, or leftover?

WeakDictionary.swift was removed, so this will probably cause issues for bootstrapping using CMake. (I thought the full test did a bootstrap, actually, but the test seems to have passed).

tomerd · 2020-10-20T18:26:29Z

@swift-ci please test

tomerd

looks great, one comment

abertelrud · 2020-10-20T18:40:42Z

I think the reference to WeakDictionary.swift in the CMake file might cause problems for bootstrapping. Apart from that I just had a suggestion for the withLock closure, but this looks great, with or without adopting that suggestion. Thanks!

abertelrud · 2020-10-20T18:41:57Z

Sources/TSCBasic/CMakeLists.txt

@@ -48,7 +48,9 @@ add_library(TSCBasic
  TerminalController.swift
  Thread.swift
  Tuple.swift
-  misc.swift)
+  misc.swift
+  WeakDictionary.swift)


WeakDictionary.swift was removed, so this will probably cause issues for bootstrapping using CMake. (I thought the full test did a bootstrap, actually, but the test seems to have passed).

tomerd · 2020-10-20T22:01:53Z

@swift-ci please test

tomerd · 2020-10-20T22:02:31Z

thanks @tgymnich 👍

abertelrud

Thanks, @tgymnich, this looks great now! I really appreciate your patience and persistence with this!

abertelrud · 2020-10-20T23:26:19Z

@tgymnich All the tests pass, so all good if I merge this at this point?

tgymnich · 2020-10-20T23:29:08Z

all good 🚀

neonichu · 2020-10-21T15:51:55Z

Thanks a lot, @tgymnich!

compnerd · 2020-10-21T20:19:49Z

This seems to be causing some issues on Windows :-(

Import WinSDK to get access to `SwitchToThread`.

tgymnich · 2020-10-22T00:17:21Z

sorry about that

TSCBasic: repair the build on Windows after #139

tgymnich requested review from abertelrud, aciidgh, friedbunny and neonichu as code owners October 6, 2020 20:21

tgymnich added 6 commits October 12, 2020 14:06

added shared and exclusive FileLocks

a13bf7e

added ReadWriteLock

506406a

added test for ReadWriteLock

2d7305f

added tests for ReadWriteFileLock

dbdcfd4

added property wrapper @ThreadLocal

fcf119d

added withLock to FileSystem

5361cfb

abertelrud assigned neonichu Oct 12, 2020

tgymnich force-pushed the move-lock-to-file-system branch from bc582aa to 5361cfb Compare October 16, 2020 23:23

tgymnich mentioned this pull request Oct 17, 2020

[SR-12851] System-wide cache of SwiftPM dependencies swiftlang/swift-package-manager#2985

Merged

tomerd reviewed Oct 19, 2020

View reviewed changes

abertelrud requested changes Oct 20, 2020

View reviewed changes

replace placeholder with actual string

202b3cf

tomerd reviewed Oct 20, 2020

View reviewed changes

tgymnich added 3 commits October 20, 2020 14:10

Move LockType inside of FileLock

c93fe44

removed WeakDictionary

df79473

Revert "added property wrapper @ThreadLocal"

5dd8686

This reverts commit fcf119d.

tgymnich requested a review from abertelrud October 20, 2020 14:10

tomerd reviewed Oct 20, 2020

View reviewed changes

abertelrud requested changes Oct 20, 2020

View reviewed changes

tgymnich added 2 commits October 20, 2020 23:04

remove WeakDictionary from CMakeLists

119ee1c

improved usage of lock

891cef9

tgymnich requested a review from abertelrud October 20, 2020 21:23

tomerd approved these changes Oct 20, 2020

View reviewed changes

tomerd assigned abertelrud and unassigned neonichu Oct 20, 2020

abertelrud approved these changes Oct 20, 2020

View reviewed changes

neonichu approved these changes Oct 21, 2020

View reviewed changes

neonichu merged commit a77e5a0 into swiftlang:main Oct 21, 2020

compnerd added a commit to compnerd/swift-tools-support-core that referenced this pull request Oct 21, 2020

TSCBasic: repair the build on Windows after swiftlang#139

560579a

Import WinSDK to get access to `SwitchToThread`.

neonichu added a commit that referenced this pull request Oct 22, 2020

Merge pull request #149 from compnerd/139

b849bfa

TSCBasic: repair the build on Windows after #139

Lock Improvements for [SR-12851] #139

Lock Improvements for [SR-12851] #139

Uh oh!

Conversation

tgymnich commented Oct 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaxDesiatov commented Oct 6, 2020

Uh oh!

MaxDesiatov commented Oct 17, 2020

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!

abertelrud Oct 20, 2020 • 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!

tgymnich Oct 20, 2020 • 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!

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!

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!

tomerd left a comment

Choose a reason for hiding this comment

Uh oh!

tomerd commented Oct 20, 2020

Uh oh!

tgymnich commented Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abertelrud commented Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abertelrud Oct 20, 2020 • 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!

tomerd commented Oct 20, 2020

Uh oh!

tomerd left a comment

Choose a reason for hiding this comment

Uh oh!

abertelrud commented Oct 20, 2020

Uh oh!

tgymnich commented Oct 6, 2020 •

edited

Loading

abertelrud Oct 20, 2020 •

edited

Loading

tgymnich Oct 20, 2020 •

edited

Loading

tgymnich commented Oct 20, 2020 •

edited

Loading

abertelrud commented Oct 20, 2020 •

edited

Loading

abertelrud Oct 20, 2020 •

edited

Loading

abertelrud Oct 20, 2020 •

edited

Loading

tomerd commented Oct 20, 2020 •

edited

Loading