Merge: Always initialize LibGit #3221
Conversation
This is required by their documentation. This certainly gives us memleaks but avoids segfaults.
| ELEKTRA_LOG ("cmerge can use libgit2 to handle arrays"); | ||
| if (handleArrays (ourCropped, theirCropped, baseCropped, result, informationKey, strategy) > 0) | ||
| { | ||
| ksDel (result); | ||
| return NULL; | ||
| } | ||
| #ifndef CMERGE_ON_LINUX | ||
| git_libgit2_shutdown (); |
There was a problem hiding this comment.
Why do we do the init but no shutdown here?
Btw if it is definitely an upstream problem/leak we can make a valgrind suppression for it and report the issue there.
There was a problem hiding this comment.
Some libraries are not safe to be initialized again after shutdown. Furthermore we do not really know if anybody else is using libgit2 in our process. So it can make sense to not call shutdown (if it is not working properly).
The whole global init and shutdown is calling for a lot of trouble. Unfortunately, many libraries do not use proper solutions (like handles).
In theory (with mutex protection and ref counting) multiple init/shutdown could be supported but it is rare that this is working properly.
and report the issue there
I agree.
There was a problem hiding this comment.
Some libraries are not safe to be initialized again after shutdown.
Ok, but the shutdown was clearly called before, and now we only call init. I assumed it was removed by mistake. If there is a reason not to call it, I'm fine with that!
There was a problem hiding this comment.
Why do we do the init but no shutdown here?
That was a mistake :/ Nonetheless, it is actually optional
Usually you don’t need to call the shutdown function as the operating system will take care of reclaiming resources, but if your application uses libgit2 in some areas which are not usually active, you can use git_libgit2_shutdown(); to ask the library to clean up the global state. (source)
Btw if it is definitely an upstream problem/leak we can make a valgrind suppression for it
I added memleak labels for both tests in this PR.
and report the issue there.
The person who answered my stackoverflow question also appears in the LibGit2 contributor list, so I assume they are aware.
There was a problem hiding this comment.
I added memleak labels for both tests in this PR.
It is better if we suppress everything from libgit. This way we still check if our code has memleaks.
The person who answered my stackoverflow question also appears in the LibGit2 contributor list, so I assume they are aware.
You never know. Better we report it. But they are obviously only interested in the latest versions, maybe they already fixed it?
Those memleak labels are already in #3209 btw.
I don't know how (yet).
This is why I'd prefer to test with their latest version first and report afterwards if necessary. @markus2330 What is higher priority: Stuff like tool integration for the merge library #3131 or this? I don't mind spending time on researching this further, but there is not too much time left until December. |
It should not segfault and should work reliable. If it does not segfault, of course tool integration is more important than valgrind supressions. So you are right, maybe having memleak label is a good solution. |
|
I created a Fedora 31 VM and compiled this branch in it. Strangely enough some tests (e.g. base64, testscr_check_formatting, testscr_check_gen IIRC) randomly fail with Therefore I am relatively sure that this PR really fixes #3220. |
|
Thank you!
If you already created the VM: Can you check if one of them always fails? (And report if it is so.)
Ok. Just to be sure: when doing the cleanup there are also problems/crashes? Otherwise we should call the cleanup. (First I thought you removed it by purpose and I gave reasons why you may have done it. But later you said you removed it by accident.) |
I haven't tested out the effects of the library function actually... IIRC I always had the shutdown when I initialized it. Until yesterday night, when I accidentally removed the shutdown while keeping the initialization.
That was unfortunate because I started to answer before your answer was posted and I did not reload. Nevertheless, both versions, shutting down and not shutting down should be OK according to the LibGit documentation. |
|
@Chemin1 thank you! Regarding the other problems: I use Fedora mainly and found no problems unique to the system. The usual suspects (dbusrecv, zeromq,..) can time out but this is not unique to Fedora. Nevertheless it might be nice to add a Fedora docker image to the tests as the package versions are usually much newer than Debian. |
Yes, I fully agree that it would be amazing to have Fedora docker images for our Jenkins tests. |
|
All the errors that I got seem to really have been random. I just got three successful |
Should fix #3206
This is required by their documentation. This certainly gives us memleaks but
avoids segfaults.
Basics
These points need to be fulfilled for every PR:
(added as entry in
doc/news/_preparation_next_release.mdwhichcontains
_(my name)_)Please always add something to the release notes.
(first line should have
module: short statementsyntax)close #X, are in the commit messages.If you have any troubles fulfilling these criteria, please write
about the trouble as comment in the PR. We will help you.
But we cannot accept PRs that do not fulfill the basics.
Checklist
Check relevant points but please do not remove entries.
For docu fixes, spell checking, and similar none of these points below
need to be checked.
(not in the PR description)
Review
Reviewers will usually check the following:
Labels
If you are already Elektra developer:
say that everything is ready to be merged.