Fix Pkg.free() so that it never removes the specified package #24048

nalimilan · 2017-10-08T14:27:30Z

The first commit adds a test which is supposed to fail on Windows only. The second commit fixes it.

However, I don't think this solves #17994, since the change just allows the package removal to complete without breaking further Pkg calls. We should probably also change Pkg.free("X") to add X to REQUIRE so that there's no risk of the package being removed by resolve, since this is what #17994 is about.

This test is supposed to fail on Windows only. Fix is coming.

On Windows, libgit2 can hold locks on files under the package directory which resolve() might want to remove, leading to corruption.

No need to duplicate code here. It turns out the multi-package method taking a collection was actually correct, as it called resolve() only after freeing libgit2 handles.

Else, resolve() will remove the package (potentially losing data), which is unlikely to be the intended effect.

nalimilan · 2017-10-08T18:37:37Z

First commit failed on Windows with the expected error. Second commit passed the tests as it should. Third commit is a more radical version which essentially does the same thing, but removes duplicated methods.

The fourth commit attempts actually fixing #17994 by always adding the specified package to REQUIRE so that it's never removed. This sounds like a good idea since it's unlikely that the user intended to remove that package by calling Pkg.free. OTOH, any package not in REQUIRE can be removed (permanently, including possibly code in git branches) at any time by resolve when doing Pkg operations, so I'm not sure this semi-fix applying only to Pkg.free is enough.

DrKrar · 2017-10-08T20:56:42Z

But wouldn't put a package in REQUIRE will make it permanent even if the user accidentally install it because it is a requirement for some package he needs?

nalimilan · 2017-10-08T21:05:42Z

Yes. But the idea is that if you call Pkg.free, you did something special with that package anyway, so you can handle it manually. You should never have to call Pkg.free on a package you don't care about.

tkelman · 2017-10-08T23:45:58Z

base/pkg/entry.jl

                end
+                return resolve()


this version isn't right either, as if the checkout fails you should restore the previous state, and not run resolve. likewise if resolve fails after checkout, should roll back.

I should have added a disclaimer that I blindly tried to fix the bug without being familiar with that code at all, so I wouldn't be surprised if that's totally incorrect. I can just note that the tests didn't cover that apparently, and that AFAICT the existing free(pkgs) method seems to work the same (but maybe I'm missing something). Anyway this commit doesn't need to be kept as removing the whole method also works.

nalimilan · 2017-10-09T07:45:42Z

@tkelman Trying to find the simplest fix which would avoid data loss until Pkg3 lands, what kind of solution to do you suggest?

tkelman · 2017-10-09T07:57:56Z

From last time I looked at this I got to about the same place you did. I think the corruption is due to libgit2 or the windows filesystem not being completely done with the checkout operation before calling the resolve

nalimilan · 2017-10-09T08:04:26Z

So do you think that the PR without the last commit is correct? It doesn't fix #17994 at all, but at least that would turn this question into a design choice (remove or not) rather than into a mix of bug+design.

tkelman · 2017-10-09T08:17:16Z

It would avoid the corruption at least, but it would also change the error recovery behavior in a way that I don't think is ideal. Is it possible to use 2 nested levels of LibGit2.transact to maybe get both without changing overall recovery behavior if resolve errors?

nalimilan · 2017-10-09T09:11:07Z

Honestly I don't understand the subtleties of that code, but AFAICT we cannot call resolve from within LibGit2.transact since it keeps file handles open. Maybe we could set a boolean flag in case of success, and only call resolve if it's set? BTW, what's the problem exactly with calling resolve after a git failure? I guess any other Pkg operation may call resolve later anyway, so if that's a problem it will bite at some later point, right?

tkelman · 2017-10-09T09:36:04Z

If something wrong with the git operation causes a failure, it should attempt to roll back to the previous state instead of leaving things in whatever in between condition they were at when the error happened.

nalimilan · 2017-10-09T12:12:35Z

If something wrong with the git operation causes a failure, it should attempt to roll back to the previous state instead of leaving things in whatever in between condition they were at when the error happened.

But the current code doesn't do that, right? I really don't want to dive into complex improvements of the Pkg system since it's going to be improved, I'm just trying to find the smallest backportable fix to avoid data loss.

tkelman · 2017-10-09T12:33:38Z

julia/base/libgit2/libgit2.jl

Lines 892 to 899 in 19b3ca7

    
           """ 
        
               transact(f::Function, repo::GitRepo) 
        
           Apply function `f` to the git repository `repo`, taking a [`snapshot`](@ref) before 
        
           applying `f`. If an error occurs within `f`, `repo` will be returned to its snapshot 
        
           state using [`restore`](@ref). The error which occurred will be rethrown, but the 
        
           state of `repo` will not be corrupted. 
        
           """

that's the intent of transact?

nalimilan · 2017-10-09T12:43:26Z

OK, I hadn't noticed the pkgs method didn't use transact. That's kind of weird. Should I just add it?

nalimilan · 2017-12-13T09:27:26Z

Not going to happen at this point.

Add test for Pkg issue #17994

f189860

This test is supposed to fail on Windows only. Fix is coming.

nalimilan changed the title ~~Add test for Pkg issue #17994~~ A first fix for Pkg.free() issue #17994 Oct 8, 2017

nalimilan force-pushed the nl/pkg branch from ec98fe9 to a7f105a Compare October 8, 2017 15:49

Only call resolve() after closing libgit2 handles in Pkg.free()

1b7f68c

On Windows, libgit2 can hold locks on files under the package directory which resolve() might want to remove, leading to corruption.

nalimilan force-pushed the nl/pkg branch from a7f105a to 1b7f68c Compare October 8, 2017 16:19

nalimilan mentioned this pull request Oct 8, 2017

Pkg.free deletes data on 0.6 RC3 #17994

Closed

Make Pkg.Entry.free(::AbstractString) call the multi-package method

782a807

No need to duplicate code here. It turns out the multi-package method taking a collection was actually correct, as it called resolve() only after freeing libgit2 handles.

kshyatt added the packages Package management and loading label Oct 8, 2017

Ensure that package is listed in REQUIRES when calling Pkg.free()

3f31162

Else, resolve() will remove the package (potentially losing data), which is unlikely to be the intended effect.

nalimilan changed the title ~~A first fix for Pkg.free() issue #17994~~ Fix Pkg.free() so that it never removes the specified package Oct 8, 2017

fredrikekre added the backport pending 0.6 label Oct 8, 2017

tkelman reviewed Oct 8, 2017

View reviewed changes

nalimilan closed this Dec 13, 2017

nalimilan deleted the nl/pkg branch December 13, 2017 09:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Pkg.free() so that it never removes the specified package #24048

Fix Pkg.free() so that it never removes the specified package #24048

nalimilan commented Oct 8, 2017 •

edited

Loading

nalimilan commented Oct 8, 2017

DrKrar commented Oct 8, 2017

nalimilan commented Oct 8, 2017

tkelman Oct 8, 2017

nalimilan Oct 9, 2017

nalimilan commented Oct 9, 2017

tkelman commented Oct 9, 2017

nalimilan commented Oct 9, 2017

tkelman commented Oct 9, 2017

nalimilan commented Oct 9, 2017

tkelman commented Oct 9, 2017

nalimilan commented Oct 9, 2017

tkelman commented Oct 9, 2017

nalimilan commented Oct 9, 2017

nalimilan commented Dec 13, 2017

Fix Pkg.free() so that it never removes the specified package #24048

Fix Pkg.free() so that it never removes the specified package #24048

Conversation

nalimilan commented Oct 8, 2017 • edited Loading

nalimilan commented Oct 8, 2017

DrKrar commented Oct 8, 2017

nalimilan commented Oct 8, 2017

tkelman Oct 8, 2017

Choose a reason for hiding this comment

nalimilan Oct 9, 2017

Choose a reason for hiding this comment

nalimilan commented Oct 9, 2017

tkelman commented Oct 9, 2017

nalimilan commented Oct 9, 2017

tkelman commented Oct 9, 2017

nalimilan commented Oct 9, 2017

tkelman commented Oct 9, 2017

nalimilan commented Oct 9, 2017

tkelman commented Oct 9, 2017

nalimilan commented Oct 9, 2017

nalimilan commented Dec 13, 2017

nalimilan commented Oct 8, 2017 •

edited

Loading