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

Provide and document @autoreleasepool block equivalent #12

Closed
lunixbochs opened this issue Feb 4, 2021 · 14 comments · Fixed by #50
Closed

Provide and document @autoreleasepool block equivalent #12

lunixbochs opened this issue Feb 4, 2021 · 14 comments · Fixed by #50
Labels
help wanted Extra attention is needed
Milestone

Comments

@lunixbochs
Copy link

From this thread: https://news.ycombinator.com/item?id=26028441

If you call into Cocoa APIs from foreign threads without an @autoreleasepool block active, the APIs will leak memory.

https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/MemoryMgmt/Articles/mmAutoreleasePools.html

Cocoa always expects code to be executed within an autorelease pool block, otherwise autoreleased objects do not get released and your application leaks memory

This StackOverflow answer suggests the @autoreleasepool block is implemented by generating calls to these objc runtime functions:

void *_objc_autoreleasePoolPush(void);
_objc_autoreleasePoolPop(void *ctxt); // ctxt is Apple's label for the param

Here's an example of how you might provide the API: https://play.golang.org/p/dljXN3BdEGr

I also suggest prominently documenting this once it is implemented, as any long running app calling into Cocoa APIs will leak without it.

@progrium
Copy link
Owner

progrium commented Feb 4, 2021

This is great, thanks again. I like your suggested API starting point.

@dolmen
Copy link

dolmen commented Feb 5, 2021

Here is an alternate Go API: the function which allocates returns the function which deallocates.

defer Autoreleasepool()()

Full code on the Go Playground.

@progrium
Copy link
Owner

progrium commented Feb 5, 2021

It worked! I like this one better. What else we got

@lunixbochs
Copy link
Author

I considered returning a function, the problem I have with that is a double paren defer doesn't seem like a common idiom in Go and it may be easier to forget or get confused about the second parens.

@lunixbochs
Copy link
Author

lunixbochs commented Feb 5, 2021

If you're already allocating a closure, you can also do this to mirror the C structure:

Autoreleasepool(func() {
    ...
})

All else the same my intuition is involving a closure may be slower in both cases.

@progrium
Copy link
Owner

progrium commented Feb 5, 2021

what does @bradfitz think of double parens? obviously you can avoid by assignment first, and its something that should only be done once so it seems silly to optimize too hard for convenience. closure is interesting too.

@lunixbochs
Copy link
Author

lunixbochs commented Feb 5, 2021

FWIW it's more correct to use autoreleasepools often, not rarely. You'd put this near the FFI boundary or around iterations of your Go event loop, not just one outside the whole program. (One autoreleasepool outside the whole program does nothing for leaks, as it won't release any objects until your program exits anyway.)

https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/MemoryMgmt/Articles/mmAutoreleasePools.html

The AppKit and UIKit frameworks process each event-loop iteration (such as a mouse down event or a tap) within an autorelease pool block

I'm also now concerned about how Go thread scheduling will impact this. I bet autoreleasepool is thread local, so it's probably unsound without LockOSThread in the loop as well (when a Goroutine hops OS threads, it will lose its autoreleasepool, or it might even migrate to a thread with a pool with a different lifetime and double free or something)

@progrium
Copy link
Owner

progrium commented Feb 5, 2021

And do I understand it correctly it only applies to objects that are sent the Autorelease message/method?

@lunixbochs
Copy link
Author

lunixbochs commented Feb 5, 2021

Yes, but you shouldn't really autorelease objects yourself, as that's very foreign to Go memory management. The reason to use an autoreleasepool in modern objc is to prevent objects that Cocoa itself autoreleases inside API calls from being leaked (and it does that all the time). It's basically an FFI boundary issue, you shouldn't cross the boundary unless you're in an autoreleasepool.

I think for your own objects, the best approach is probably runtime.SetFinalizer([object release]) for any APIs that return non-borrowed objects.

@progrium progrium added this to the 0.1.0 milestone Feb 5, 2021
@progrium
Copy link
Owner

progrium commented Feb 5, 2021

@lunixbochs thank you for explaining. can you expand on the setfinalizer example? it sounds like i will need to document this.

@lunixbochs
Copy link
Author

lunixbochs commented Feb 5, 2021

Here you call Alloc and return, so the "caller owns" the resulting object and must eventually Release it:

https://github.com/progrium/macdriver/blob/main/cocoa/NSImage.go#L32-L34

func NSImage_InitWithData(data core.NSData) NSImage {
    return NSImage{objc.Get("NSImage").Alloc().Send("initWithData:", data)}
}

Many objc APIs also return caller owned data, such as NSImage_imageNamed().

You can opt into the Go GC instead by doing something like this (needs further work as GC example doesn't return anything, but it's the general idea)

func (obj *objc.Object) GC() {
    runtime.SetFinalizer(obj, (*objc.Object).Release)
}

func NSImage_InitWithData(data core.NSData) NSImage {
    return NSImage{objc.Get("NSImage").Alloc().Send("initWithData:", data)}.GC()
}

This means instead of:

img := NSImage_InitWithData(data)
defer img.Release()

You'd just do this:

img := NSImage_InitWithData(data)

Then the Go GC will automatically release objects when they have no references remaining.

However it does look like you're currently passing objects as structs by value (containing a uintptr), which probably means the Go GC isn't involved. To fix that you'd need to use pointer types. Using the Go GC to manage objc lifetimes would be more memory safe than the current system. The current setup makes use after free and double free trivial, which IMO defeats a lot of the purpose of using Go in the first place.

This is off topic from this issue, it's probably worth discussing in a new issue if you're interested.

@progrium
Copy link
Owner

progrium commented Feb 6, 2021

Yes! Would love a new thread on that idea, it sounds right.

@progrium
Copy link
Owner

progrium commented Feb 9, 2021

It would be hugely helpful if somebody could write a test that will fail until a solution for this is merged.

@progrium progrium pinned this issue Feb 14, 2021
@progrium progrium added the help wanted Extra attention is needed label Mar 8, 2021
@progrium progrium mentioned this issue Mar 21, 2021
2 tasks
@progrium progrium unpinned this issue Mar 21, 2021
@progrium progrium modified the milestones: 0.1.0, 0.2.0 Mar 30, 2021
@mgood
Copy link
Sponsor Collaborator

mgood commented May 14, 2021

I wonder if it would be reasonable for Send to allocate an autoreleasepool by default. So after each objc call it would automatically release any autoreleased objects.
For performance-sensitive code I'm not sure what the overhead would be. Though I'm still trying to think through what patterns would work to specify that some block of calls should take place within the caller's autoreleasepool. Calling obj.Autorelease() would imply that an object should be in a pool, so maybe that could return an Object where Send doesn't create new pools. But something more explicit could be better.

LockOSThread does sound like it should be used with the autoreleasepools.

@mgood mgood mentioned this issue May 20, 2021
@mgood mgood closed this as completed in #50 Oct 21, 2021
mgood added a commit that referenced this issue Oct 21, 2021
Implements `Autorelease(func())` to run the body with an active autoreleasepool.

Existing calls to `NSAutoreleasePool` have been replaced and wrapped with this block helper.

Fixes #12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants