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

[SR-74] Passing reference of uninitialized lazy to C function leads to memory issues during deinit #42696

Open
swift-ci opened this issue Dec 5, 2015 · 23 comments
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself

Comments

@swift-ci
Copy link
Collaborator

swift-ci commented Dec 5, 2015

Previous ID SR-74
Radar None
Original Reporter etan (JIRA User)
Type Bug

Attachment: Download

Environment

OS X 10.11.1 (15B42)
Xcode Version 7.1.1 (7B1005)
Swift version 2.1 (700.1.101.6 700.1.76)

Additional Detail from JIRA
Votes 0
Component/s Compiler
Labels Bug
Assignee None
Priority Medium

md5: 45a7d69794999ab90b93c6c258f5fb14

Issue Description:

To understand this issue, a C API is necessary with two C functions. One to store a function and an info pointer - a second one to call the stored function with the stored pointer as argument.

typedef void (* CFuncCallback)(void * info);
void CFuncStore(CFuncCallback callback, void * info);
void CFuncCall();

static CFuncCallback _callback;
static void * _info;

void CFuncStore(CFuncCallback callback, void * info) {
    _callback = callback;
    _info = info;
}

void CFuncCall() {
    if (_callback) {
        _callback(_info);
    }
}

A simple info object and C callback function are implemented in Swift to access this C API.

final class Info {
    init() { print("Info: init.") }
    deinit { print("Info: deinit.") }
}

func cCallback(info: UnsafeMutablePointer<Void>) {
    print("Callback from C.")
    print(UnsafePointer<Info>(info).memory)
    print("Test successful - no crash.")
}

Now, a Swift object is defined.

  • On init(), an Info instance is created and registered with the C API.

  • On deinit, invoke CFuncCall is invoked. This should be fine, as the Swift object still holds a reference to the info object while deinit is in progress.

final class SwiftObj {
    var info: Info = {
        print("SwiftObj: Alloc info.")
        return Info()
    }()

    init() {
        print("SwiftObj: init.")
        CFuncStore(cCallback, &info);
        print("SwiftObj: init done.")
    }
    
    deinit {
        print("SwiftObj: deinit.")
        CFuncCall()
        print("SwiftObj: deinit done.")
    }
}

An instance of SwiftObj is created and immediately destroyed, producing this log:

@lilyball
Copy link
Mannequin

lilyball mannequin commented Dec 6, 2015

On deinit, invoke CFuncCall is invoked. This should be fine, as the Swift object still holds a reference to the info object while deinit is in progress.

I don't actually think this is supposed to be ok. Once a Swift object has entered deinit, no other code should have access to the object.

Also, storing a pointer to &info is wildly unsafe. The semantics of the Swift language only guarantee that the pointer that is passed to CFuncStore is valid for the duration of the function call. Once CFuncStore returns, the pointer is no longer valid. This is because the compiler is free to use a pointer to a local stack value (this allows it to support inout with computed properties, but it's free to do it at any time for other values too). There's a compiler optimization that makes inout parameters actually be pointers to the original underlying storage when possible, but the documentation (I forget where, probably in the book) explicitly states that you must not rely on this optimization, because it is not guaranteed and because it's not part of the defined language semantics.

@swift-ci
Copy link
Collaborator Author

swift-ci commented Dec 6, 2015

Comment by Etan Kissling (JIRA)

Thanks for the comment!

First of all, passing &info works fine, as the example shows.
The only thing that's broken is that if &info points to a lazily initialized variable, things get messy.
Lazy initialization should not influence how &info behaves after initialization.

The rest of this comment addresses your concerns about the validity of the issue.


Once a Swift object has entered deinit, no other code should have access to the object.

Yes, and my example does not violate this. Only &info is passed to the C API; the C API doesn't know anything about the SwiftObj that is being deinited.

During deinit of the SwiftObj, its properties (including the info variable) are still valid, allowing you to clean up unmanaged resources.

  1. Last reference to SwiftObj is nilled

    • SwiftObj.release drops the reference count of SwiftObj to 0.

    • info is still at +1

  2. SwiftObj.deinit is called.

    • info is still at +1
  3. SwiftObj.deinit calls the C API that uses the stored reference to info.

    • That's fine! info is still at +1.

    • The C API does not know anything about SwiftObj – only about info

  4. SwiftObj.deinit completes

  5. All objects strongly referenced by SwiftObj receive a release call.

    • info.releaseinfo is now at reference count 0
  6. info.deinit

  7. info.free

  8. SwiftObj.free


Also, storing a pointer to &info is wildly unsafe. [...] This is because the compiler is free to use a pointer to a local stack value [...]

Could you please quote relevant sections of sources that support this statement?

While I also lack any official reference that describes how exactly Swift's memory model works, I wrote down the practical implications that your statement would have:

  • If a C / ObjC API stores a pointer beyond the duration of the function call, there is no single way to reliably invoke it from Swift.

  • Recent improvements to Swift extended compatibility with C APIs by adding support for C callbacks via @convention(c). The prioritization of such improvements wouldn't make sense if simple, popular C APIs cannot be used.

  • Swift lacks keywords like C# fixed to annotate variables so that they are pinned in memory. If objects could move around in memory, such keywords would be required when objects are passed into unmanaged APIs.

@lilyball
Copy link
Mannequin

lilyball mannequin commented Dec 6, 2015

The fact that it appears to work in practice does not mean that it actually works according to the language semantics. Just look at C, where there's practically infinite ways to write code that works in practice with current compilers but actually invokes undefined behavior. The semantics of Swift explicitly state that when passing an inout reference to a function that takes an UnsafePointer or UnsafeMutablePointer, the pointer is only valid for the duration of the function call. The fact that things are broken if &info is a lazy property is just a demonstration of the fact that you're violating language semantics, and is not actually a bug in Swift.

Once a Swift object has entered deinit, no other code should have access to the object.

Yes, and my example does not violate this.

Yes it does. You're passing a pointer into the memory of SwiftObj, and then accessing that pointer from within deinit. You're not accessing the actual SwiftObj type, but you are accessing memory that's managed by SwiftObj, and that's the same thing.

During deinit of the SwiftObj, its properties (including the info variable) are still valid, allowing you to clean up unmanaged resources.

Not strictly true. They're valid and can be accessed from your code inside of deinit, but you're not accessing info from inside of deinit, you're accessing it from a C callback that Swift doesn't know anything about. The compiler is free to actually manage the underlying resources however it wants as long as it preserves the language semantics. So in this case it's free to clean up the info variable before even executing deinit, since your deinit doesn't access that property. Now, I'm not sure what is actually going on in this case, because you say that initializing the lazy property in init before passing it to the callback actually works around the issue, which implies that it's not a matter of deinit cleaning it up early. This may actually be a demonstration of copy-in copy-out being used instead of call by reference when handing the pointer to the C callback. I'd have to debug this to find out and I haven't done that, because I don't think it really matters why it's failing when the root cause is a violation of language semantics.

Also, storing a pointer to &info is wildly unsafe. [...] This is because the compiler is free to use a pointer to a local stack value [...]

Could you please quote relevant sections of sources that support this statement?

Certainly. The Swift Programming Language, Language Reference, Declarations, In-Out Parameters:

In-out parameters are passed as follows:

1. When the function is called, the value of the argument is copied.
2. In the body of the function, the copy is modified.
3. When the function returns, the copy’s value is assigned to the original argument.

This behavior is known as copy-in copy-out or call by value result. For example, when a computed property or a property with observers is passed as an in-out parameter, its getter is called as part of the function call and its setter is called as part of the function return.

As an optimization, when the argument is a value stored at a physical address in memory, the same memory location is used both inside and outside the function body. The optimized behavior is known as call by reference; it satisfies all of the requirements of the copy-in copy-out model while removing the overhead of copying. Do not depend on the behavioral differences between copy-in copy-out and call by reference.

(emphasis mine)

If a C / ObjC API stores a pointer beyond the duration of the function call, there is no single way to reliably invoke it from Swift.

It's not that there is "no reliable way", it's that it's strictly illegal to dereference that pointer after the function ends if it was created from an inout reference. If you need a pointer that lasts longer, you need to actually alloc it on the heap and manage the memory.

This includes popular APIs like the KVO pattern [...]

Not really; in all my years of doing Mac/iOS programming, I've never seen the kvoContext parameter used as an actual pointer to anything. It's always just used with strict pointer comparison in order to figure out if the class responding to the event was the one that registered it. This is demonstrated by your own link, which uses the following as an example:

static void * XXContext = &XXContext;

Here the pointed-to data is irrelevant. It's using its own pointer as its value simply in order to guarantee uniqueness. And the actual usage of this value in the code is with a simple pointer equality comparison:

if (context == XXContext) {

Recent improvements to Swift extended compatibility with C APIs by adding support for C callbacks via @convention(c). The prioritization of such improvements wouldn't make sense if simple, popular C APIs cannot be used.

I fail to see how the semantics of inout parameters has any bearing on C compatibility. There's nothing about C compatibility that requires the ability to pass pointers into the memory managed by a particular value. If you want to pass a pointer into a C function that it needs to hold on to and dereference later, you're free to allocate your own memory with `UnsafeMutablePointer.alloc` and use that.

Swift lacks keywords like C# fixed to annotate variables so that they are pinned in memory. If objects could move around in memory, such keywords would be required when objects are passed into unmanaged APIs.

Swift doesn't have a garbage collector. And even the deprecated Obj-C garbage collector wasn't a compacting garbage collector, which means the notion of "fixed" doesn't even apply to the language (either Swift or Obj-C).

@swift-ci
Copy link
Collaborator Author

swift-ci commented Dec 7, 2015

Comment by Etan Kissling (JIRA)

I really appreciate the insightful discussion going on here! Much better than the black hole that bugreport.apple.com was ^^

Indeed, your reference to the specification explaining how inout parameters should be assumed to work make this issue fall into the category of undefined behaviour.


That said, I still want to find a solution to the original problem, that is, passing a pointer to a Swift object into a C API that survives the immediate function call.

Even for the KVO pattern, when it's used like everyone uses it, it's broken. Because of copy-in copy-out behaviour, one has to assume that &kvoContext == &kvoContext does not always hold, as the compiler is free to allocate the copy to different locations for each reference.

deinit { removeObserver(self, forKeyPath: "somePath", context: &kvoContext) }
init() { addObserver(self, forKeyPath: "somePath", options: [.New, .Old, .Initial], context: &kvoContext) }

override func observeValueForKeyPath(
    keyPath: String?,
    ofObject object: AnyObject?,
    change: [String : AnyObject]?,
    context: UnsafeMutablePointer<Void>
) {
    guard &kvoContext == context else { return }
    // Handle event.
}

In the example of KVO, one could generate a random number into an instance variable, then unsafebitcast it to a void *, then pass that in as the context. This way, the context stays stable for the lifetime of the object – as opposed to the duration of the individual function calls.


A better example of C APIs where such a trick cannot be applied is a selection of CoreFoundation APIs (e.g. the FSEvents API).
In these APIs, a C callback function is first registered together with a context object. Then, the C callback is invoked later as events arrive. The initially registered context object is passed as an argument so that it is possible to identify to the corresponding object.

struct CContext {
    var version: CFIndex
    var info: UnsafeMutablePointer<Void>
    var retain: CFAllocatorRetainCallBack?
    var release: CFAllocatorReleaseCallBack?
    var copyDescription: CFAllocatorCopyDescriptionCallBack?
}

typealias CCallback = @convention(c) (obj: CObjRef, info: UnsafeMutablePointer<Void>, /* args */) -> Void

func CObjCreate(
    allocator: CFAllocator?,
    _ callback: CCallback,
    _ context: UnsafeMutablePointer<CContext>,
    /* args */
) -> CObjRef

func CCObjRelease(obj: CObjRef)
  • CObjCreate allocates CObjRef, then copies CContext, then calls CContext.retain(CContext.info).

  • When events arrived, CCallback is called with CContext.info.

  • CCObjRelease calls CContext.release(CContext.info), then frees CObjRef and the CContext copy.

==> How can we safely get back to our Swift object from a @convention(c) callback?

One really hackish workaround could involve maintaining a Dictionary of all CInfo instances keyed with a randomly generated UInt, and passing the Dictionary key to the C API. This way, we could look up our CInfo, and from there, via a weak reference, check whether SwiftObj is still there. This method would essentially be the same as my suggested KVO pattern above, where the problem is worked around by not storing pointers but instead storing simple {{Int}}s. Would such a solution be safe?

@swift-ci
Copy link
Collaborator Author

swift-ci commented Dec 7, 2015

Comment by Etan Kissling (JIRA)

BTW: There is something like C# fixed with withUnsafeMutablePointer(_:_:).

I'll try building an example with UnsafeMutablePointer.alloc - not sure if I'll like it – the Dictionary hack described at the bottom of my last comment is at least something that does not have unsafe written all over it 🙂

@swift-ci
Copy link
Collaborator Author

swift-ci commented Dec 7, 2015

Comment by Etan Kissling (JIRA)

I've attached an example that uses UnsafeMutablePointer.alloc. Do you approve of this method? 🙂

SelfManagedMemory.zip

@lilyball
Copy link
Mannequin

lilyball mannequin commented Dec 7, 2015

You are correct in that the common usage of KVO is technically in violation. It is rather unfortunate. Also, withUnsafeMutablePointer doesn't solve anything, all it does is convert the inout parameter to a pointer for the duration of the block, but it's the same pointer that you'd be passing if you just relied on the implicit conversion done when passing inout references to functions that take pointers.

Using UnsafeMutablePointer.alloc is technically the only safe way to pass an arbitrary context pointer under your control to a C function. If all you need is pointer identity, the other option is to init some object and use unsafeAddressOf() on the object to get its pointer. Or if you want to pass an object itself, you can use Unmanaged which can do pointer conversions.

Regarding your source example, I think you're doing more work than necessary. You've basically reinvented Unmanaged here. If you want to have a class Info where you pass a pointer in to the callbacks, you can just use Unmanaged itself, which can convert to/from COpaquePointer, which can convert to/from UnsafePointer:

final class Info {
    weak var swiftObj: SwiftObj? // unowned, if CFuncRelease() won't touch this
    init() {}
}

final class SwiftObj {
    let cFuncRef: CFuncRef
    init() {
        // we can create the Info now and then set the swiftObj ref after
        // this lets us skip the IOW on the property
        var info = Info()
        var context = CFuncContext(
            info: UnsafePointer(Unmanaged.passUnretained(info).toOpaque()),
            retain: { Unmanaged<Info>.fromOpaque(COpaquePointer($0)).retain() },
            release: { Unmanaged<Info>.fromOpaque(COpaquePointer($0)).release() }
        )
        cFuncRef = CFuncCreate(&context)
        // now that we've initialized our properties, we can refer to self
        info.swiftObj = self
    }

    deinit {
        CFuncRelease(cFuncRef)
    }
}

(note: I haven't actually tried running this)

@belkadan
Copy link
Contributor

belkadan commented Dec 8, 2015

I can confirm that at this time the only pointers guaranteed to be valid passed the lifetime of a call are global stored variables without observers. Class properties without accessors are "likely" to "work" but are not guaranteed to. Lazy properties almost certainly will not work, since they are implemented with accessors under the hood.

Either Unmanaged or a manually-managed UnsafeMutablePointer<MyStruct> is the right way to go for this pattern.

@lilyball
Copy link
Mannequin

lilyball mannequin commented Dec 8, 2015

I can confirm that at this time the only pointers guaranteed to be valid passed the lifetime of a call are global stored variables without observers.

Is this actually part of the language semantics? The Swift book doesn't list any exceptions to the rule saying that you should not rely on the difference between copy-in copy-out and call by reference. If we're ok with committing to global properties without accessors working like this, it would be great to have that documented (and it sounds like a good idea too, as that fixes the issue with the common practice of KVO).

Class properties without accessors are "likely" to "work" but are not guaranteed to.

How about static properties on structs?

@swift-ci
Copy link
Collaborator Author

swift-ci commented Dec 8, 2015

Comment by Etan Kissling (JIRA)

I've built a working solution based on Unmanaged now. Looks much cleaner than UnsafeMutablePointer.

import Cocoa

@NSApplicationMain
final class AppDelegate: NSObject, NSApplicationDelegate {
    func applicationDidFinishLaunching(notification: NSNotification) {
        _ = SwiftObj()
    }
}
final class Info<T: AnyObject> {
    private(set) weak var object: T?
    
    init(_ object: T) {
        print("Info: init -- Swift object reference: \(object).")
        self.object = object
    }
    
    deinit {
        print("Info: deinit -- Swift object reference: \(object).")
    }
}
final class SwiftObj: CustomStringConvertible {
    private typealias InfoType = Info<SwiftObj>
    
    private var cFuncRef: CFuncRef!

    init() {
        print("SwiftObj: init.")
        
        withExtendedLifetime(Info(self)) { (info: InfoType) in
            print("SwiftObj: Creating context.")
            var context = CFuncContext(
                info: UnsafePointer(Unmanaged.passUnretained(info).toOpaque()),
                retain: {
                    print("Callback: retain.")
                    return UnsafePointer(Unmanaged<InfoType>.fromOpaque(COpaquePointer($0)).retain().toOpaque())
                },
                release: {
                    print("Callback: release.");
                    Unmanaged<InfoType>.fromOpaque(COpaquePointer($0)).release()
                }
            )
            
            print("SwiftObj: Creating CFunc.")
            cFuncRef = CFuncCreate(&context)
        }
        
        print("SwiftObj: init done.")
    }
    
    deinit {
        print("SwiftObj: deinit.")
        CFuncRelease(cFuncRef)
        print("SwiftObj: deinit done.")
    }
    
    var description: String {
        return "<SwiftObj: description>"
    }
}

producing this log:

@lilyball
Copy link
Mannequin

lilyball mannequin commented Dec 8, 2015

Info may be accessed from a different thread than the one that contains the SwiftObj reference (i.e. a separate NSRunLoop).

Bear in mind that Swift doesn't have any knowledge of concurrency or atomicity, so if you're accessing the object from two threads concurrently you have a problem. I hope that the actual access pattern here properly accesses it from one and only one thread at any given moment.

Info.object must be weak, not unowned.

Strictly speaking, if you can guarantee that the Info is cleaned up without ever accessing its object property before your deinit finishes, then it would be safe to make it unowned. But if, as you say, the C API can actually clean it up later, then yeah, you want weak, otherwise the unowned reference can outlive the referenced object, and that's bad (it's technically legal in Swift as long as you never actually access that reference again, except that if the referenced object inherits from NSObject then there's a bug (which I assume still exists) that causes a security hazard anyway, due to how unowned works on Obj-C objects).

withExtendedLifetime must probably be used to ensure that Info stays alive until ownership transfer.

Good idea. Alternatively, you could create the Unmanaged with passRetained, construct the CFuncRef, and then call release() on the Unmanaged (this way the Unmanaged guarantees that it keeps the Info alive during setup), but it's probably simpler to just use withExtendedLifetime as you did.

Using a generic factory to produce retain and release callbacks leads to Segmentation fault: 11 during IR code generation.

Ouch! If this isn't already documented somewhere, you should file a bug on it. Either the compiler should disallow it, or it should compile, it certainly shouldn't crash.

Or is it safe to use Unmanaged<Info> instead of Unmanaged<InfoType>?

Since Info takes a type parameter, you can't write Unmanaged<Info>.

Should we generally assume copy-in copy-out when the & operator is used?

I think the answer to that is "yes". The inout keyword isn't being used, but the & operator is still an inout reference, and using one that coerces to an UnsafePointer is effectively just sugar for wrapping the function call in a call to withUnsafePointer.

@swift-ci
Copy link
Collaborator Author

swift-ci commented Dec 8, 2015

Comment by Etan Kissling (JIRA)

I hope that the actual access pattern here properly accesses it from one and only one thread at any given moment.

Yep, if there are multiple threads, proper synchronization is required e.g. via NSOperationQueue when accessing Info. Therefore, Info can be owned by a different thread than SwiftObj. Even without concurrency, the C API could use an NSOperationQueue internally that first works through queued events before finally cleaning up the whole state asynchronously. weak is just the safer option here, and the overhead vs. unowned is minimal.

If this isn't already documented somewhere, you should file a bug on it.

Yeah, Segmentation fault: 11 and SourceKit crashes are unfortunately so common for me, and always vanish very easily by simply changing the program flow a bit. Building minimal samples to reliably reproduce these issues is quite cumbersome – and even if you report these, they are typically fixed very slowly. Different topic, though.

Are generic @convention(c ) callbacks really not supported, or did I just not try hard enough? I don't see why they should not be implementable.

Since Info takes a type parameter, you can't write Unmanaged<Info>.

You can write Unmanaged<Info> in the example and it still compiles - however, I'm not sure what exactly this does in that context 🙂
I also fail to see why a retain / release action needs to know anything about the type at all (except that it's operating on AnyObject).

Seems like we have a stable solution now, though![]( Hopefully we can get official confirmation by @belkadan that this is indeed the way to go, so that the pattern can be pushed into the various code bases. Then, the crusade to update all Google results that distribute incorrect KVO pattern information can also begin)

@lilyball
Copy link
Mannequin

lilyball mannequin commented Dec 8, 2015

You can write Unmanaged<Info> in the example and it still compiles - however, I'm not sure what exactly this does in that context

I just tested and I get an error if I change Unmanaged<InfoType> to Unmanaged<Info>. I used your previously-listed class Info and class SwiftObj definitions, along with a pure-Swift reimplementation of the CFuncRef API: https://gist.github.com/kballard/58b7dd24ac6eb32d8c5b

I also fail to see why a retain / release action needs to know anything about the type at all (except that it's operating on AnyObject).

It doesn't really, but the Unmanaged type exposes other methods that give you back the wrapped value (if it didn't, it wouldn't be very useful), and those methods do need to know the type of the underlying object.

@swift-ci
Copy link
Collaborator Author

swift-ci commented Dec 9, 2015

Comment by Etan Kissling (JIRA)

About Unmanaged<Info>

Try this class:

final class Info<T: AnyObject> {
    private(set) weak var object: T?
    
    init(_ object: T) { self.object = object }
    
    deinit { print("Info: deinit -- Swift object reference: \(object).") }
    
    class func retain(info: UnsafePointer<Void>) -> UnsafePointer<Void> {
        return UnsafePointer(Unmanaged<Info>.fromOpaque(COpaquePointer(info)).retain().toOpaque())
    }
}

This compiles although using Unmanaged<Info> without a generic type specifier. Can't pass it to the C API this way, though.

@lilyball
Copy link
Mannequin

lilyball mannequin commented Dec 9, 2015

That compiles because it's inside of the class definition, and references to a generic type from inside that type's definition typically assumes you want the same type parameters that self has. You'd still have to call it like Info<SwiftObj>.retain(ptr).

@swift-ci
Copy link
Collaborator Author

swift-ci commented Dec 9, 2015

Comment by Etan Kissling (JIRA)

I see 🙂 Makes perfectly sense.

So, the only thing missing is an official confirmation that the pattern posted above is okay, then.

@belkadan
Copy link
Contributor

belkadan commented Dec 9, 2015

I really don't like the explicit calls to retain and release. That's what passRetainedValue is for. The ultimate release could be done using an explicit release, but even then I'd prefer a takeRetainedValue to let Swift take care of it.

"Is the global variable thing actually part of the language semantics?" No, it is not—we don't actually have a formal memory model yet. (And we're not likely to have that discussion until we're ready to talk about concurrency.) That said, I can promise we won't break using a global stored variable (in the same module) as a KVO context until we have such a model, and most likely it won't break then either. We've recommended this pattern on the Apple developer forums before.

@swift-ci
Copy link
Collaborator Author

swift-ci commented Dec 9, 2015

Comment by Etan Kissling (JIRA)

Thanks for the clarifications on the global variable behaviour 🙂 This means that we simply have to promote kvoContexts from instance variables to global variables - which should actually be fine (as they are only used to distinguish whether an event is directed at the own class or the super class).

It's also very nice to hear that concurrency specification is somewhere on the roadmap!


I'm a bit confused about your dislike for retain / release, though. You suggest passRetainedValue instead of retain, and takeRetainedValue instead of release – semantically, these two operations are the same, afaik, except that the actual Swift object is unwrapped in the first case and needs to be rewrapped again or the unused result warning suppressed. However, the code becomes significantly uglier if the first method is used:

var context = CFuncContext(
    info: UnsafePointer(Unmanaged.passUnretained(info).toOpaque()),
    retain: { UnsafePointer(Unmanaged.passRetained(Unmanaged<InfoType>.fromOpaque(COpaquePointer($0)).takeUnretainedValue()).toOpaque()) },
    release: { _ = Unmanaged<InfoType>.fromOpaque(COpaquePointer($0)).takeRetainedValue() }
)

instead of

var context = CFuncContext(
    info: UnsafePointer(Unmanaged.passUnretained(info).toOpaque()),
    retain: { UnsafePointer(Unmanaged<InfoType>.fromOpaque(COpaquePointer($0)).retain().toOpaque()) },
    release: { Unmanaged<InfoType>.fromOpaque(COpaquePointer($0)).release() }
)

If your comment relates to the actual callbacks in the C API: yeah, I agree that they are painful to use, but can't change those, unfortunately 🙁
C APIs like FsEvents that use this pattern typically raise events on a separate NSRunLoop, so it is important that ownership of Info is fully transferred to the C API, and that the C API can decide when it wants to call retain and release. The timings when these memory management callbacks are invoked is not documented – therefore, it's up to the C API to decide when it no longer needs access to the Info. This happens independently of the SwiftObj lifecycle in the example (which is fine).

Obviously, in the minimal example posted above where everything is single threaded and we know that the C API completes synchronously, we could simply link the lifetime of SwiftObj, the Info and the C API together, merge Info with SwiftObj, and come up with this simple implementation.

final class SwiftObj {
    private var cFuncRef: CFuncRef!
    
    init() {
        var context = CFuncContext(
            info: UnsafePointer(Unmanaged.passUnretained(self).toOpaque()),
            retain: nil,
            release: nil
        )

        cFuncRef = CFuncCreate(&context)
    }
    
    deinit {
        CFuncRelease(cFuncRef)
    }
}

@belkadan
Copy link
Contributor

belkadan commented Dec 9, 2015

I guess you're right about FSEvents, but from what I've seen the common case for C APIs is merely a single destructor parameter, like pthread_key_create. In these cases having to manually do retain and release is more error-prone.

I still get the feeling that your solution is way overcomplicated even for FSEvents. I'll come back and comment more on that later.

@lilyball
Copy link
Mannequin

lilyball mannequin commented Dec 9, 2015

If the API exposes a single destructor parameter, I agree that passRetainedValue and takeRetainedValue are better. But when it exposes explicit retain/release callbacks I agree with Etan that trying to use passRetainedValue or takeRetainedValue to mimic retain/release is very awkward and it's better to just call the retain / release directly.

@belkadan
Copy link
Contributor

Okay, it took longer than I meant it to to come back to this. Here's what I came up with, deliberately avoiding the need for a box class:

let retainCallback: @convention(c) (UnsafePointer<Void>) -> UnsafePointer<Void> = {
  Unmanaged<AnyObject>.fromOpaque(COpaquePointer($0)).retain()
  return $0
}

let releaseCallback: @convention(c) (UnsafePointer<Void>) -> Void = {
  Unmanaged<AnyObject>.fromOpaque(COpaquePointer($0)).release()
}

extension FSEventStreamContext {
  init(object: AnyObject) {
    self.version = 0
    // This will be improved by making toOpaque return UnsafePointer.
    self.info = UnsafeMutablePointer(Unmanaged.passUnretained(object).toOpaque())
    self.retain = retainCallback
    self.release = releaseCallback
    self.copyDescription = { _ = $0; fatalError("something annoying to deal with the +1 return") }
  }

  var infoObject: AnyObject {
    return Unmanaged.fromOpaque(COpaquePointer(self.info)).takeUnretainedValue()
  }
}

// Example
let obj = NSObject()
withExtendedLifetime(obj) {
  let context = FSEventStreamContext(object: obj)
  print(context.infoObject)
  // Actually do something here.
  // withExtendedLifetime is still necessary because there's no initial retain when the context struct is created.
}

@swift-ci
Copy link
Collaborator Author

Comment by Etan Kissling (JIRA)

Taking longer is fine :-) The answer timing is still way better than I'm used to in general. You're doing a great job here![]( Thanks again for looking into this)

A few comments about your proposal:

  • In your retain and release callback you use Unmanaged<AnyObject>. Is this safe? Since this is not a type safe part, I was rather cautious with not specifying the exact type that's being represented.

    • If this is correct, that's actually really nice, since it removes the need for generic retain and release callbacks.
  • Nice to see that you agree with passing a dedicated NSObject as info and not self (or the owner of the FSEventStreamRef). Although it's probably safe to pass self in the case of FSEvents, it is not documented that FSEventStreamRelease releases info synchronously.

    • Probably it's still nicer to pass something non-objc, i.e. an instance of an empty class definition. We are dealing with a CoreFoundation API here that shouldn't know anything about the objc world.

    • Or promote AnyObject from protocol to class to make it instantiable while still being the implicit root class of every object. Maybe something for swift-evolution.

  • retainCallback: I have no idea why we have to return UnsafePointer<Void> in the CFAllocatorRetainCallback, but retain() actually returns a pointer, so I bet it's safer to repackage that pointer with passUnretained().toOpaque() and return that instead of $0 unless there is some documentation that states that retain() always returns the same thing as was input (which would actually make sense to me)
  • copyDescription: Hm. I've got no idea when this is ever used. In the snippet below, I simply pass a +1 reference to String(info.object), hoping that +1 is correct because the callback is named copy____.
  • Using a plain NSObject works here in the simplified example, but you still need the box class if you actually want to use the full FSEvents API.

    • For the simplified sample with only retain & release, you could get rid of obj as well, and simply pass nil as info, retain, release and copyDescription.

    • Problem is to get back to the Swift object from the C callback registered in FSEventStreamCreate. That's where I'm using the box object with the weak pointer in order to get back to the Swift world.

I've attached my proposal for the full FSEvents API usage (non-minified): FSEvents.zip
After launching, click Start, then change the contents of ~/Downloads to see the events being produced on the Xcode console.

Here are the relevant code parts:

private final class Info<T: AnyObject> {
    private(set) weak var object: T?
    
    init(_ object: T) {
        self.object = object
    }
}

final class FileSystemWatcher {
    private typealias InfoType = Info<FileSystemWatcher>
    
    private let rootPath: String
    
    private lazy var eventStream: FSEventStreamRef = {
        return withExtendedLifetime(Info(self)) { (info: InfoType) in
            var context = FSEventStreamContext(
                version: 0,
                info: UnsafeMutablePointer(Unmanaged.passUnretained(info).toOpaque()),
                retain: { UnsafePointer(Unmanaged<InfoType>.fromOpaque(COpaquePointer($0)).retain().toOpaque()) },
                release: { Unmanaged<InfoType>.fromOpaque(COpaquePointer($0)).release() },
                copyDescription: {
                    let object = Unmanaged<InfoType>.fromOpaque(COpaquePointer($0)).takeUnretainedValue().object
                    return Unmanaged<CFString>.passRetained(String(object))
                }
            )
            
            return FSEventStreamCreate(
                /* allocator: */ nil,
                /* callback: */ { streamRef, clientCallBackInfo, numEvents, eventPaths, eventFlags, eventIds in
                    let info = Unmanaged<InfoType>.fromOpaque(COpaquePointer(clientCallBackInfo)).takeUnretainedValue()
                    guard let object = info.object else { return }
                    
                    let paths = Unmanaged<CFArrayRef>.fromOpaque(COpaquePointer(eventPaths)).takeUnretainedValue()
                        as NSArray as! [CFStringRef] as! [String]
                    
                    for i in 0 ..< numEvents {
                        object.eventStream(
                            streamRef,
                            didOutputEventForPath: paths[i],
                            flags: eventFlags[i],
                            id: eventIds[i]
                        )
                    }
                },
                /* context: */ &context,
                /* pathsToWatch: */ [self.rootPath],
                /* sinceWhen: */ FSEventStreamEventId(kFSEventStreamEventIdSinceNow),
                /* latency: */ 0.01,
                /* flags: */ FSEventStreamCreateFlags(0 | // The 0 | here is just to work around Xcode formatter bug.
                    kFSEventStreamCreateFlagUseCFTypes |
                    kFSEventStreamCreateFlagNoDefer |
                    kFSEventStreamCreateFlagWatchRoot |
                    kFSEventStreamCreateFlagIgnoreSelf |
                    kFSEventStreamCreateFlagFileEvents
                )
            )
        }
    }()
    
    init(rootPath: String) {
        self.rootPath = (rootPath as NSString).stringByStandardizingPath
        FSEventStreamScheduleWithRunLoop(eventStream, CFRunLoopGetCurrent(), kCFRunLoopDefaultMode)
    }
    
    deinit {
        stop()
        FSEventStreamUnscheduleFromRunLoop(eventStream, CFRunLoopGetCurrent(), kCFRunLoopDefaultMode)
        FSEventStreamInvalidate(eventStream)
        FSEventStreamRelease(eventStream)
    }

    private var started = false
    
    func start() {
        guard !started else { return } // If you start multiple time, API misuse warning appears.
        started = true
        FSEventStreamStart(eventStream)
    }
    
    func stop() {
        guard started else { return }
        started = false
        FSEventStreamStop(eventStream)
    }

    private func eventStream(
        eventStream: ConstFSEventStreamRef,
        didOutputEventForPath path: String,
        flags: FSEventStreamEventFlags,
        id: FSEventStreamEventId)
    {
        guard eventStream == self.eventStream else { return }

        // Process event.
        let allFlags = [
            kFSEventStreamEventFlagNone: "None",
            kFSEventStreamEventFlagMustScanSubDirs: "MustScanSubDirs",
            kFSEventStreamEventFlagUserDropped: "UserDropped",
            kFSEventStreamEventFlagKernelDropped: "KernelDropped",
            kFSEventStreamEventFlagEventIdsWrapped: "EventIdsWrapped",
            kFSEventStreamEventFlagHistoryDone: "HistoryDone",
            kFSEventStreamEventFlagRootChanged: "RootChanged",
            kFSEventStreamEventFlagMount: "Mount",
            kFSEventStreamEventFlagUnmount: "Unmount",
            kFSEventStreamEventFlagItemCreated: "ItemCreated",
            kFSEventStreamEventFlagItemRemoved: "ItemRemoved",
            kFSEventStreamEventFlagItemInodeMetaMod: "ItemInodeMetaMod",
            kFSEventStreamEventFlagItemRenamed: "ItemRenamed",
            kFSEventStreamEventFlagItemModified: "ItemModified",
            kFSEventStreamEventFlagItemFinderInfoMod: "ItemFinderInfoMod",
            kFSEventStreamEventFlagItemChangeOwner: "ItemChangeOwner",
            kFSEventStreamEventFlagItemXattrMod: "ItemXattrMod",
            kFSEventStreamEventFlagItemIsFile: "ItemIsFile",
            kFSEventStreamEventFlagItemIsDir: "ItemIsDir",
            kFSEventStreamEventFlagItemIsSymlink: "ItemIsSymlink"
            ].map { (FSEventStreamEventFlags($0.0), $0.1) }
        
        print("========================================================================")
        print("FSEvent ID: \(id)")
        print(" - Path: \(path)")
        print(" - Flags: \(allFlags.filter { flags & $0.0 != 0 }.map { $0.1 })")
        let unknownFlags = allFlags.reduce(flags) { $0 & ~$1.0 }
        if unknownFlags != 0 {
            print(" - Unknown flags: \(unknownFlags))")
        }
    }
}

@Dante-Broggi
Copy link
Contributor

Has this been resolved? If so, it should be closed.

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself
Projects
None yet
Development

No branches or pull requests

3 participants