[WIP] Use a managed HTTP(S) transport implementation #374

2017-04-17T13:26:09-05:00

carlosmn commented

2017-04-17 13:26:09 -05:00

(Migrated from github.com)

This is a bit like #372 but goes further by replacing the whole HTTP stack and just leaving the logic in libgit2. This lets us use the Go HTTP code as well, instead of the one in the library.

The current state is enough to let the tests work but there is no custom proxy configuration available and we don't currently test that authentication actually works.

Proxy support depends on libgit2/libgit2#4206 or similar getting merged. I guess we'll have to add poxyproxy to our tests as well so it doesn't regress.

While this is useful by itself (by taking us completely away from OpenSSL and libcurl in libgit2), a potentially more exciting outcome might be to build on the lessons from this to build a managed ssh transport, which would eliminate the need for libssh2.

This is a bit like #372 but goes further by replacing the whole HTTP stack and just leaving the logic in libgit2. This lets us use the Go HTTP code as well, instead of the one in the library. The current state is enough to let the tests work but there is no custom proxy configuration available and we don't currently test that authentication actually works. Proxy support depends on libgit2/libgit2#4206 or similar getting merged. I guess we'll have to add poxyproxy to our tests as well so it doesn't regress. While this is useful by itself (by taking us completely away from OpenSSL and libcurl in libgit2), a potentially more exciting outcome might be to build on the lessons from this to build a managed ssh transport, which would eliminate the need for libssh2.

AaronO commented

2017-06-26 10:40:41 -05:00

(Migrated from github.com)

@carlosmn This is pretty exciting, I actually hacked on implementing pure-go transports a few months ago. Would be great to reduce the surface area of C code and potential memory leaks.

We've had issues with libgit2 memory leaks on long running daemons, the go heap is rarely over 10MB but the process' resident memory grows to gigabytes.

I also explored a more radical approach and hacked together a fairly useable git2go compatible lib called go2git that wraps https://github.com/src-d/go-git (so it's 100% go with no C code).

@carlosmn This is pretty exciting, I actually hacked on implementing pure-go transports a few months ago. Would be great to reduce the surface area of C code and potential memory leaks. We've had issues with `libgit2` memory leaks on long running daemons, the `go` heap is rarely over `10MB` but the process' resident memory grows to gigabytes. I also explored a more radical approach and hacked together a fairly useable `git2go` compatible lib called `go2git` that wraps https://github.com/src-d/go-git (so it's 100% go with no C code).

lhchavez (Migrated from github.com) reviewed 2019-01-05 18:32:28 -06:00

http.go

													
				@ -0,0 +417,4 @@

					header.Data = uintptr(unsafe.Pointer(data))

					n, err := stream.Read(p)

					if err != nil {

lhchavez (Migrated from github.com) commented

2019-01-05 18:32:28 -06:00

Per https://golang.org/pkg/io/#Reader, this should be

	if n == 0 && err != nil {

Per https://golang.org/pkg/io/#Reader, this should be ```suggestion if n == 0 && err != nil { ```

lhchavez commented

2019-01-05 18:43:42 -06:00

(Migrated from github.com)

you should also be able to add something along the lines of

diff --git a/script/build-libgit2-static.sh b/script/build-libgit2-static.sh
index 680dd93..a17944a 100755
--- a/script/build-libgit2-static.sh
+++ b/script/build-libgit2-static.sh
@@ -12,6 +12,8 @@ cd "${BUILD_PATH}/build" &&
 cmake -DTHREADSAFE=ON \
       -DBUILD_CLAR=OFF \
       -DBUILD_SHARED_LIBS=OFF \
+      -DUSE_HTTPS=OFF \
+      -DUSE_EXT_HTTP_PARSER=OFF \
       -DCMAKE_C_FLAGS=-fPIC \
       -DCMAKE_BUILD_TYPE="RelWithDebInfo" \
       -DCMAKE_INSTALL_PREFIX="${BUILD_PATH}/install" \

you should also be able to add something along the lines of ```diff diff --git a/script/build-libgit2-static.sh b/script/build-libgit2-static.sh index 680dd93..a17944a 100755 --- a/script/build-libgit2-static.sh +++ b/script/build-libgit2-static.sh @@ -12,6 +12,8 @@ cd "${BUILD_PATH}/build" && cmake -DTHREADSAFE=ON \ -DBUILD_CLAR=OFF \ -DBUILD_SHARED_LIBS=OFF \ + -DUSE_HTTPS=OFF \ + -DUSE_EXT_HTTP_PARSER=OFF \ -DCMAKE_C_FLAGS=-fPIC \ -DCMAKE_BUILD_TYPE="RelWithDebInfo" \ -DCMAKE_INSTALL_PREFIX="${BUILD_PATH}/install" \ ```

lhchavez commented

2019-01-15 19:15:10 -06:00

(Migrated from github.com)

Hi! I did a small refactoring of this change and

a) split it between creating a Transport interface and implementing the HTTP/HTTPs transports
b) did what you mentioned in the first commit and did the SSH transport

with that, libgit2 can be compiled without openssh and openssl! with a bit more work, we can probably make it fully static.

you can find the refactor here: https://github.com/lhchavez/git2go/commits/transports what would be the best course of action to move this change forward?

Hi! I did a small refactoring of this change and a) split it between creating a Transport interface and implementing the HTTP/HTTPs transports b) did what you mentioned in the first commit and did the SSH transport with that, libgit2 can be compiled without openssh and openssl! with a bit more work, we can probably make it _fully_ static. you can find the refactor here: https://github.com/lhchavez/git2go/commits/transports what would be the best course of action to move this change forward?

lhchavez commented

2021-09-05 20:31:38 -05:00

(Migrated from github.com)

I just landed https://github.com/libgit2/git2go/pull/810, which was heavily based upon this change, with proxy support!