[swift-evolution] [swift-evolution-announce] [Review] SE-0159: Fix Private Access Levels

David Hart david at hartbit.com
Thu Mar 23 01:47:14 CDT 2017


Hi Drew Crawford,

I understand that scoped private can be of value, and that code is a good example. But I never argued that private was useless (it would never have been proposed and accepted in the first case if it was). I just argue that the advantage is not worth its presence in the language. Here’s my take:

3. This file cannot be made an entire module, because it's actually used as part of several projects that are shipped as frameworks, and apparently some developers get really annoyed when they have to drag too many frameworks into their Xcode project.

This fails to convince me. If this piece of code is really used in several projects, I think it belongs in its own module and I would have implemented it by moving ThreadsafeWrapper to its own file, thus keeping the same thread-safety guarantees and hiding of your current example. The feels to me like this piece of code is using private only because it is shying away from using module-scope. Once we gain submodules in the future, it would be even easier to move both classes to their own sub-module.

David.

> On 23 Mar 2017, at 06:17, Drew Crawford via swift-evolution <swift-evolution at swift.org> wrote:
> 
> It has been expressed in various ways "does anybody actually use scoped visibility in the wild" and "what real benefit does it provide to production code".
> 
> The below file or some estranged stepchild of it appears at least 5 repositories (that I know of; this is practically a samizdat around here).  The scoped access modifier is a significant improvement to the safety of this code and by extension the many projects that contain it.
> 
> Apologies if this listing is rather tiring.  Many of the "solutions" proposed by the anti-private camp sound great in a sentence but fall apart at scale.  But it is not obvious why this is so to people who do not use the keyword, so I can understand why they keep suggesting poor solutions.  Perhaps with a more involved listing, it will become more obvious why many of these suggestions do not work.  The original is a lot longer; I did reduce it to only the parts that seem relevant to this thread.
> 
> import Foundation
> 
> /**
>  This code demonstrates one of the usecases for 'private'.  Adapted from real production code, this file exports a threading primitive to the rest of the program.
>  
>  To state it briefly, private is necessary here because we need the following visibility nesting to achieve our objective:
>  
>  ┌───────────────────────────────────────────┐
>  │PROGRAM/internal                           │
>  │                                           │
>  │                                           │
>  │     ┌───────────────────────────────────┐ │
>  │     │ ThreadsafeWrapperNotifyChanged    │ │
>  │     │                                   │ │
>  │     │      ┌──────────────────────┐     │ │
>  │     │      │   ThreadsafeWrapper  │     │ │
>  │     │      │                      │     │ │
>  │     │      │                      │     │ │
>  │     │      │          value       │     │ │
>  │     │      │                      │     │ │
>  │     │      │                      │     │ │
>  │     │      └──────────────────────┘     │ │
>  │     │                                   │ │
>  │     └───────────────────────────────────┘ │
>  └───────────────────────────────────────────┘
>  
>  In particular:
>  
>  1.  value a.k.a. "t" must be protected against all potential unsafe access.  This file is hundreds of lines, auditing the whole thing is very tedious.
>  2.  ThreadsafeWrapper is an implementation detail of ThreadsafeWrapperNotifyChanged which is not intended for use by other callers.  To avoid exposing the class to other callers, it must appear in this file.
>  3.  This file cannot be made an entire module, because it's actually used as part of several projects that are shipped as frameworks, and apparently some developers get really annoyed when they have to drag too many frameworks into their Xcode project.
>  4.  The use of `private` here reduces the "maybe not threadsafe" part of the code from 196 lines to 47 lines (a reduction of buggy code of 76%).  In the production file from which this example is derived, the reduction is from 423 lines to 33 lines, or 92%.  A scoped access variable significantly improves the threadsafety of this code.
> 
>  */
> 
> //Define an interface common to both major components
> private protocol Threadsafe : class {
>     ///the type of the value we are protecting
>     associatedtype T
>     ///Access the underlying value.
>     ///- parameter block: The block that will be passed the protected value.  The block acts as an exclusive lock; while you're in it, no other consumers will be accessing the value.  
>     ///- complexity: Coalescing multiple operations into a single block improves performance.
>     func accessT<K>(_ block: @escaping (T) throws -> K) throws -> K
>     ///Mutate the underlying value.
>     ///- parameter block: The block that will be passed the protected value.  The block acts as an exclusive lock; while you're in it, no other consumers will be accessing the value.
>     ///- complexity: Coalescing multiple operations into a single block improves performance.
>     func mutateT<K>(_ block: @escaping (inout T) throws -> K) throws -> K
> }
> 
> ///Some convenience accessors for callers that do not need a block-based API to get lock semantics / operation coalescing
> extension Threadsafe {
>     var value : T {
>         get {
>             var t: T! = nil
>             try! accessT({ lt in
>                 t = lt
>             })
>             return t
>         }
>         set {
>             try! mutateT({ (lt:inout T) -> () in
>                 lt = newValue
>             })
>         }
>     }
> }
> 
> ///The core synchronization primitive.  This is a private implementation detail of ThreadsafeWrapperNotifyChanged.
> //MARK: audit this area begin
> private final class ThreadsafeWrapper<T> : Threadsafe {
>     /**The value we are protecting.  This value needs to be protected against unsafe access from
>     1.  This type, if a scoped keyword is available (as shown)
>     2.  The entire file, if a scoped keyword is removed.
>      Only access this value on the synchronizationQueue.
>      */
>     private var t: T
>     ///The queue that is used to synchronize the value, only access the value from the queue.
>     ///- note: fileprivate is used here to allow the wrapped object to access the queue.  Demonstrating that both are legitimately useful modifiers.
>     fileprivate let synchronizationQueue: DispatchQueue
>     
>     internal init (t: T, queueDescription: String) {
>         self.synchronizationQueue = DispatchQueue(label: "foo")
>         self.t = t
>     }
>     
>     //MARK: implement our Threadsafe protocol
>     func accessT<K>(_ block: @escaping (T) throws -> K) throws -> K {
>         var k : K!
>         var err: Error?
>         synchronizationQueue.sync() {[unowned self] () -> Void in
>             do { try k = block(self.t) }
>             catch { err = error }
>         }
>         if let err = err { throw err }
>         return k
>     }
>     
>     func mutateT<K>(_ block: @escaping (inout T) throws -> K) throws -> K {
>         var k : K!
>         var err: Error?
>         synchronizationQueue.sync() {[unowned self] () -> Void in
>             do { k = try self.fastMutateT(block) }
>             catch { err = error }
>         }
>         if let err = err { throw err }
>         return k
>     }
>     
>     ///An alternate mutation function that can only be used when inside a block already.
>     ///- note: Calling this function from the wrong queue is NOT thread-unsafe, it will merely crash the program.  So exposing this API to the file may introduce bugs, but none of them are a threadsafety concern.
>     func fastMutateT<K>(_ block: @escaping (inout T) throws -> K) throws -> K {
>         dispatchPrecondition(condition: .onQueue(synchronizationQueue))
>         return try block(&t)
>     }
> }
> //MARK: audit area end
> 
> /**Like ThreadsafeWrapper, but also allows us to find out when the wrapped object changes.
>  For that reason, it has a little more overhead than ThreadsafeWrapper, and requires the wrapped type to be equatable */
> final class ThreadsafeWrapperNotifyChanged<T: Equatable> : Threadsafe {
>     
>     ///Hold the value and a list of semaphores
>     private let tsw: ThreadsafeWrapper<(T, [DispatchSemaphore])>
>     
>     internal init (t: T, queueDescription: String) {
>         self.tsw = ThreadsafeWrapper(t: (t, []), queueDescription: "foo")
>     }
>     
>     //MARK: implement our Threadsafe protocol
>     func mutateT<K>(_ block: @escaping (inout T) throws -> K) throws -> K {
>         var k : K!
>         try tsw.mutateT { v in
>             defer {
>                 for sema in v.1 {
>                     sema.signal()
>                 }
>             }
>             try self.tsw.fastMutateT({ v in
>                 try block(&v.0)
>             })
>         }
>         return k
>     }
>     func accessT<K>(_ block: @escaping (T) throws -> K) throws -> K {
>         return try tsw.accessT({ v -> K in
>             return try block(v.0)
>         })
>     }
>     
>     
>     /**Notify when the value passed in has changed or the timeout has expired.
>      By passing a particular value, we can avoid many race conditions.*/
>     func waitForChange(oldValue: T, timeOut: TimeInterval) throws {
>         var sema : DispatchSemaphore! = nil
>         if oldValue != tsw.value.0 { return } //fastpath
>         
>         //slowpath
>         try accessT {[unowned self] (tee) -> () in
>             if oldValue != tee { return }
>             sema = DispatchSemaphore(value: 0)
>             try self.tsw.fastMutateT({ v in
>                 v.1.append(sema)
>             })
>         }
>         if sema == nil { return }
>         //clean up semaphore again
>         defer {
>             try! tsw.mutateT { v in
>                 v.1.removeItemMatchingReference(sema)
>             }
>         }
>         let time = DispatchTime.now() + timeOut
>         if sema.wait(timeout: time) == .timedOut { throw Errors.DeadlineExceeded }
>         //now, did we change?
>         let changed = try accessT { (val) -> Bool in
>             return val != oldValue
>         }
>         if changed { return }
>         try waitForChange(oldValue: oldValue, timeOut: timeOut) //🐞
>     }
> }
> 
> //MARK: utility
> enum Errors: Error {
>     case DeadlineExceeded
> }
> 
> extension RangeReplaceableCollection where Iterator.Element : AnyObject {
>     /// Remove first colleciton element that matches the given reference
>     mutating func removeItemMatchingReference(_ object : Iterator.Element) {
>         if let index = self.index(where: {$0 === object}) {
>             self.remove(at: index)
>         }
>     }
> }
> 
> On March 20, 2017 at 6:54:55 PM, Douglas Gregor (dgregor at apple.com <mailto:dgregor at apple.com>) wrote:
> 
>> Hello Swift community,
>> 
>> The review of SE-0159 "Fix Private Access Levels" begins now and runs through March 27, 2017. The proposal is available here:
>> 
>> https://github.com/apple/swift-evolution/blob/master/proposals/0159-fix-private-access-levels.md <https://github.com/apple/swift-evolution/blob/master/proposals/0159-fix-private-access-levels.md>
>> Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-evolution mailing list at
>> 
>> https://lists.swift.org/mailman/listinfo/swift-evolution <https://lists.swift.org/mailman/listinfo/swift-evolution>
>> or, if you would like to keep your feedback private, directly to the review manager. When replying, please try to keep the proposal link at the top of the message:
>> 
>> Proposal link:
>> 
>> https://github.com/apple/swift-evolution/blob/master/proposals/0159-fix-private-access-levels.md <https://github.com/apple/swift-evolution/blob/master/proposals/0159-fix-private-access-levels.md>
>> Reply text
>> Other replies
>>  <https://github.com/apple/swift-evolution/blob/master/process.md#what-goes-into-a-review-1>What goes into a review?
>> 
>> The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:
>> 
>> What is your evaluation of the proposal?
>> Is the problem being addressed significant enough to warrant a change to Swift?
>> Does this proposal fit well with the feel and direction of Swift?
>> If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
>> How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
>> More information about the Swift evolution process is available at
>> 
>> https://github.com/apple/swift-evolution/blob/master/process.md <https://github.com/apple/swift-evolution/blob/master/process.md>
>> Thank you,
>> 
>> -Doug
>> 
>> Review Manager
>> 
>> _______________________________________________
>> swift-evolution-announce mailing list
>> swift-evolution-announce at swift.org <mailto:swift-evolution-announce at swift.org>
>> https://lists.swift.org/mailman/listinfo/swift-evolution-announce <https://lists.swift.org/mailman/listinfo/swift-evolution-announce>
> _______________________________________________
> swift-evolution mailing list
> swift-evolution at swift.org <mailto:swift-evolution at swift.org>
> https://lists.swift.org/mailman/listinfo/swift-evolution <https://lists.swift.org/mailman/listinfo/swift-evolution>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.swift.org/pipermail/swift-evolution/attachments/20170323/2a5703bf/attachment.html>


More information about the swift-evolution mailing list