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

KeepAlive all the things #393

Merged
merged 5 commits into from
Jul 8, 2017
Merged

KeepAlive all the things #393

merged 5 commits into from
Jul 8, 2017

Conversation

carlosmn
Copy link
Member

@carlosmn carlosmn commented Jul 8, 2017

Newer Go runtimes' GC is particularly eager and doesn't even consider the receiver as a reference. Thus in order for structs wrapping the pointers not to be freed while in the middle of us using them in a function call, we must make sure that the wrapping struct is kept alive until after we return from.

This seems to be the cause of #352, #334 and #356 and this should fix the other places where this might happen.

carlosmn and others added 5 commits July 7, 2017 23:24
…eivers

We can't work on the copies here, we need to have pointer receivers so we know
we're keeping alive the object whose finalizer would free the unmanaged memory
we're working with.
@carlosmn carlosmn merged commit 08db2e2 into master Jul 8, 2017
@carlosmn carlosmn deleted the cmn/keepalive-all-the-things branch July 8, 2017 18:49
@odeke-em
Copy link

odeke-em commented Sep 4, 2017

@carlosmn thank you for working on this, for all the work and maintaining git2go!

I am checking in, having been paged in by @josharian who is currently on leave and away from his computer. By Newer Go runtimes' GC what would the older version be? I ask because the spread of versions in the closed issues is diverse, ranging from Go1.6 to Go1.8Beta1

Issue GoVersion
#352 Go1.6
#334 Unspecified
#356 Go1.7.4 and Go1.8Beta1

@carlosmn
Copy link
Member Author

carlosmn commented Sep 5, 2017

AFAICT it was Go 1.6 which introduced the more eager garbage collector, but each version brings its own GC changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants