Browse Source

Ensure BlameReaders close at end of request (#12102)

#11716 reports multiple git blame processes hanging around
this was thought to be due to timeouts, however on closer look this
appears to be due to the Close() function of the BlameReader hanging
with a blocked stdout pipe.

This PR fixes this Close function to:

* Cancel the context of the cmd
* Close the StdoutReader - ensuring that the output pipe is closed

Further it makes the context of the `git blame` command a child of the
request context - ensuring that even if Close() is not called, on
cancellation of the Request the blame is command will also be cancelled.

Fixes #11716
Closes #11727

Signed-off-by: Andrew Thornton <art27@cantab.net>
for-closed-social
zeripath 4 years ago
committed by GitHub
parent
commit
858c35b731
No known key found for this signature in database GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 13 additions and 8 deletions
  1. +8
    -6
      modules/git/blame.go
  2. +4
    -1
      modules/git/blame_test.go
  3. +1
    -1
      routers/repo/blame.go

+ 8
- 6
modules/git/blame.go View File

@ -79,7 +79,9 @@ func (r *BlameReader) NextPart() (*BlamePart, error) {
// Close BlameReader - don't run NextPart after invoking that // Close BlameReader - don't run NextPart after invoking that
func (r *BlameReader) Close() error { func (r *BlameReader) Close() error {
defer process.GetManager().Remove(r.pid) defer process.GetManager().Remove(r.pid)
defer r.cancel()
r.cancel()
_ = r.output.Close()
if err := r.cmd.Wait(); err != nil { if err := r.cmd.Wait(); err != nil {
return fmt.Errorf("Wait: %v", err) return fmt.Errorf("Wait: %v", err)
@ -89,19 +91,19 @@ func (r *BlameReader) Close() error {
} }
// CreateBlameReader creates reader for given repository, commit and file // CreateBlameReader creates reader for given repository, commit and file
func CreateBlameReader(repoPath, commitID, file string) (*BlameReader, error) {
func CreateBlameReader(ctx context.Context, repoPath, commitID, file string) (*BlameReader, error) {
gitRepo, err := OpenRepository(repoPath) gitRepo, err := OpenRepository(repoPath)
if err != nil { if err != nil {
return nil, err return nil, err
} }
gitRepo.Close() gitRepo.Close()
return createBlameReader(repoPath, GitExecutable, "blame", commitID, "--porcelain", "--", file)
return createBlameReader(ctx, repoPath, GitExecutable, "blame", commitID, "--porcelain", "--", file)
} }
func createBlameReader(dir string, command ...string) (*BlameReader, error) {
// FIXME: graceful: This should have a timeout
ctx, cancel := context.WithCancel(DefaultContext)
func createBlameReader(ctx context.Context, dir string, command ...string) (*BlameReader, error) {
// Here we use the provided context - this should be tied to the request performing the blame so that it does not hang around.
ctx, cancel := context.WithCancel(ctx)
cmd := exec.CommandContext(ctx, command[0], command[1:]...) cmd := exec.CommandContext(ctx, command[0], command[1:]...)
cmd.Dir = dir cmd.Dir = dir
cmd.Stderr = os.Stderr cmd.Stderr = os.Stderr

+ 4
- 1
modules/git/blame_test.go View File

@ -5,6 +5,7 @@
package git package git
import ( import (
"context"
"io/ioutil" "io/ioutil"
"testing" "testing"
@ -93,8 +94,10 @@ func TestReadingBlameOutput(t *testing.T) {
if _, err = tempFile.WriteString(exampleBlame); err != nil { if _, err = tempFile.WriteString(exampleBlame); err != nil {
panic(err) panic(err)
} }
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
blameReader, err := createBlameReader("", "cat", tempFile.Name())
blameReader, err := createBlameReader(ctx, "", "cat", tempFile.Name())
if err != nil { if err != nil {
panic(err) panic(err)
} }

+ 1
- 1
routers/repo/blame.go View File

@ -123,7 +123,7 @@ func RefBlame(ctx *context.Context) {
return return
} }
blameReader, err := git.CreateBlameReader(models.RepoPath(userName, repoName), commitID, fileName)
blameReader, err := git.CreateBlameReader(ctx.Req.Context(), models.RepoPath(userName, repoName), commitID, fileName)
if err != nil { if err != nil {
ctx.NotFound("CreateBlameReader", err) ctx.NotFound("CreateBlameReader", err)
return return

Loading…
Cancel
Save