go-git-http icon indicating copy to clipboard operation
go-git-http copied to clipboard

Process open on connection close

Open rande opened this issue 9 years ago • 4 comments

Hello,

There is a bug with this library (or with the way I implemented it). When a clone is stopped (ctrl+z) during the upload-pack operation, the upload-pack command remains in memory.

See demo with the upload-pack: bug_githttp

The project is available here: https://github.com/rande/pkgmirror/blob/master/mirror/git/git_app.go#L77-L161

rande avatar Oct 17 '16 16:10 rande

This might comes from those lines: https://github.com/AaronO/go-git-http/blob/master/githttp.go#L116-L120

    // Copy input to git binary
    io.Copy(stdin, rpcReader)
    stdin.Close()

    // Write git binary's output to http response
    io.Copy(w, gitReader)

    // Wait till command has completed
    mainError := cmd.Wait()

I will try to use a proper io.Pipe struct and check for connection errors.

rande avatar Oct 17 '16 16:10 rande

Ok, get it, now I have a proper error: INFO[0010] Git server error commit=185599c733a573cb312bcc37ef2b94fbf31476bd dir=/mirror/data/git/github.com/sonata-project/sandbox-build.git error=write tcp 127.0.0.1:8000->127.0.0.1:49615: write: broken pipe type=fetch

I will send a PR.

rande avatar Oct 17 '16 20:10 rande

Related PR: https://github.com/AaronO/go-git-http/pull/26

rande avatar Oct 17 '16 21:10 rande

@rande Good catch, thanks for reporting this !

I think it's clear that we need to have robust error handling there.

One question, does this also happen if you SIGKILL or SIGTERM the process (e.g: CTRL+C) ?

I took a look at your PR ( #26 ), it adds better error handling but the fact that we're creating functions on the fly likely points out that we should abstract all this into a struct with those functions as methods.

I'm gonna get this fixed and version with semver at the same time (since we use glide too), I'm not 100% sure I'll merge your PR yet, since I might go the struct route instead.

Eitherway thanks for the issue and PR !

AaronO avatar Dec 14 '16 12:12 AaronO