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

Don't free() the handle after dlclose #140

Merged
merged 1 commit into from
Apr 15, 2015

Conversation

garrison
Copy link
Contributor

Of the possible code paths, there was only one that freed the handle after calling dlclose. This change removes that free() statement, fixing an issue that was recently brought to light by JuliaLang/julia#10808.

Of the possible code paths, there was only one that freed the handle
after calling dlclose.  This change removes that free() statement,
fixing an issue that was recently brought to light by
JuliaLang/julia#10808.
ihnorton added a commit that referenced this pull request Apr 15, 2015
Don't free() the handle after dlclose
@ihnorton ihnorton merged commit aac14b4 into JuliaPackaging:master Apr 15, 2015
@tkelman
Copy link
Contributor

tkelman commented Apr 15, 2015

Should this be conditional on VERSION?

@garrison
Copy link
Contributor Author

Sure, this introduces a (very small) memory leak for old VERSIONs, but there were already code paths in BinDeps that leaked the same memory. So I don't think it's worth adding a conditional on VERSION.

@garrison garrison deleted the double_free branch April 15, 2015 19:09
@garrison
Copy link
Contributor Author

What would people think of doing a BinDeps release now that this change is merged? Right now the released version of BinDeps doesn't jive with julia master (see e.g. @jiahao's encounter with this bug).

@tkelman
Copy link
Contributor

tkelman commented Apr 16, 2015

Are we positive this is the right fix, before or after JuliaLang/julia#10832 or equivalent gets merged? I'm very very hesitant to change any behavior of BinDeps on release Julia, since it's critical infrastructure, either directly or indirectly, for such a large portion of packages. If BinDeps was compensating for a memory leak in base and we're not going to backport the change to base (and if we did, we'd have to bump REQUIRE here to reflect that minimum version of Julia), I'd rather leave that in place.

@garrison
Copy link
Contributor Author

There were two symptoms in BinDeps that resulted from JuliaLang/julia#10808 being merged. The first resulted from JuliaLang/julia@11a0f43, the fix for which is in JuliaLang/julia#10832 (still waiting to be merged, though I am sure this is the correct fix).

The other resulted from JuliaLang/julia@fcf4a47, which is fixed in this pull request. Yes, BinDeps was compensating for a memory leak in base, though not fully -- there are in fact two other calls to Libc.dlclose in dependencies.jl which were not accompanied by a call to Libc.free, each of which (before and after this pull request) leaks two pointers of memory on Julia 0.3.

@mlubin
Copy link
Contributor

mlubin commented Apr 16, 2015

Is this memory leak just applicable during Pkg.build? It seems pretty harmless, I'd rather have travis working again for NLopt/Cbc on Julia nightly.

@tkelman
Copy link
Contributor

tkelman commented Apr 16, 2015

I agree it's not good to leave this broken on nightly for too long. If the memory leak on 0.3 is small and only occurs during Pkg.build then I guess it's not that bad, but "there were already leaks elsewhere" doesn't seem like a very good reason to knowingly introduce new leaks.

@ihnorton
Copy link
Member

I vote to add a conditional for 0.3 only (don't worry about 0.4-dev sub-versions), and then tag BinDeps.

@garrison
Copy link
Contributor Author

I concur with @ihnorton.

@tkelman
Copy link
Contributor

tkelman commented Apr 17, 2015

That works for me.

@ihnorton
Copy link
Member

Done.

@tkelman
Copy link
Contributor

tkelman commented Apr 17, 2015

didn't help for long, tuple changes caused more breakage

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.

4 participants