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

Allow use of libgit2 built without thread support #918

Closed
wants to merge 1 commit into from
Closed

Allow use of libgit2 built without thread support #918

wants to merge 1 commit into from

Conversation

pjbgf
Copy link

@pjbgf pjbgf commented Jul 13, 2022

Allow use of libgit2 built without thread support.

With smart transport, users using solely Managed Transports can rely on Go to deal with the multi-threading requirements of transport management.

To enable threadless libgit2 an environment variable ALLOW_THREADLESS_LIBGIT2 must be set to true.

As long as the git2go objects are not being shared across Go routines this arrangement seems to work well.

By leveraging this and the removal of unmanaged transport we have experienced a decrease in segfaults or random errors when using git2go for accessing multiple Git repositories concurrently.

Fixes #917.

git.go Outdated
Comment on lines 146 to 154
// Due to the multithreaded nature of Go and its interaction with
// calling C functions, we cannot work with a library that was not built
// with multi-threading support. The most likely outcome is a segfault
// or panic at an incomprehensible time, so let's make it easy by
// panicking right here.
if features&FeatureThreads == 0 {
panic("libgit2 was not built with threading support")
}

Copy link
Contributor

@lhchavez lhchavez Jul 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think we can really remove this unless we make something that forces git2go calls to be in the same OS thread (or add a huge mutex lock surrounding every CGO call).

at least not in general. some applications might be totally fine with this. but this is a library, and needs to support that case.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lhchavez thank you for the quick reply. The main issue we have is that this validation happens at init(), which blocks downstream projects from going around it without forking git2go.

Would you be able to recommend any other approach in which we could fix #917 instead? We have been investigating user reports for about 3 months, as it was quite difficult to get to the right conditions to reproduce, so we would really appreciate any other recommendations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lhchavez would it be possible to add an env variable or a global variable to bypass this check? The default behavior would still be to run the check and panic, if required. But for projects, which would like to use libgit2 without multithreading support, it'd be nice to have a way of disabling this panic.

Copy link
Author

@pjbgf pjbgf Jul 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I amended the PR accordingly, please take another look.

With smart transport, users using solely Managed Transports can rely on
Go to deal with the multi-threading requirements of transport
management.

To enable threadless libgit2 an environment variable
ALLOW_THREADLESS_LIBGIT2 must be set to true.

As long as the git2go objects are not being shared across Go routines
this arrangement seems to work well.

By leveraging this and the removal of unmanaged transport we have
experienced a decrease in segfaults or random errors when using git2go
for accessing multiple Git repositories concurrently.

Fixes #917.

Signed-off-by: Paulo Gomes <[email protected]>
@pjbgf pjbgf closed this by deleting the head repository Nov 22, 2022
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.

Segfault when calling index.AddAll
3 participants