From 0ee28ec8341f817b3d5c0b67c0fec4154613ab73 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Sun, 13 Feb 2022 21:09:03 +0000 Subject: [PATCH 1/3] Prevent dangling GetAttribute calls It appears possible that there could be a hang due to unread data from the repo-attribute command pipes. This PR simply closes these during the defer. Signed-off-by: Andrew Thornton --- services/gitdiff/gitdiff.go | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index fb3e32935dd16..babe7be77a5fd 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -190,9 +190,11 @@ var ( codeTagSuffix = []byte(``) ) -var unfinishedtagRegex = regexp.MustCompile(`<[^>]*$`) -var trailingSpanRegex = regexp.MustCompile(`]?$`) -var entityRegex = regexp.MustCompile(`&[#]*?[0-9[:alpha:]]*$`) +var ( + unfinishedtagRegex = regexp.MustCompile(`<[^>]*$`) + trailingSpanRegex = regexp.MustCompile(`]?$`) + entityRegex = regexp.MustCompile(`&[#]*?[0-9[:alpha:]]*$`) +) // shouldWriteInline represents combinations where we manually write inline changes func shouldWriteInline(diff diffmatchpatch.Diff, lineType DiffLineType) bool { @@ -206,7 +208,6 @@ func shouldWriteInline(diff diffmatchpatch.Diff, lineType DiffLineType) bool { } func fixupBrokenSpans(diffs []diffmatchpatch.Diff) []diffmatchpatch.Diff { - // Create a new array to store our fixed up blocks fixedup := make([]diffmatchpatch.Diff, 0, len(diffs)) @@ -658,10 +659,10 @@ func (diffFile *DiffFile) GetTailSection(gitRepo *git.Repository, leftCommitID, LastRightIdx: lastLine.RightIdx, LeftIdx: leftLineCount, RightIdx: rightLineCount, - }} + }, + } tailSection := &DiffSection{FileName: diffFile.Name, Lines: []*DiffLine{tailDiffLine}} return tailSection - } func getCommitFileLineCount(commit *git.Commit, filePath string) int { @@ -942,8 +943,8 @@ parsingLoop: // TODO: There are numerous issues with this: // - we might want to consider detecting encoding while parsing but... // - we're likely to fail to get the correct encoding here anyway as we won't have enough information - var diffLineTypeBuffers = make(map[DiffLineType]*bytes.Buffer, 3) - var diffLineTypeDecoders = make(map[DiffLineType]*encoding.Decoder, 3) + diffLineTypeBuffers := make(map[DiffLineType]*bytes.Buffer, 3) + diffLineTypeDecoders := make(map[DiffLineType]*encoding.Decoder, 3) diffLineTypeBuffers[DiffLinePlain] = new(bytes.Buffer) diffLineTypeBuffers[DiffLineAdd] = new(bytes.Buffer) diffLineTypeBuffers[DiffLineDel] = new(bytes.Buffer) @@ -1416,10 +1417,12 @@ func GetDiff(gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff if err != nil && err != ctx.Err() { log.Error("Unable to open checker for %s. Error: %v", opts.AfterCommitID, err) } + _ = checker.Close() cancel() }() } defer func() { + _ = checker.Close() cancel() }() } @@ -1539,7 +1542,8 @@ func GetWhitespaceFlag(whiteSpaceBehavior string) string { "ignore-all": "-w", "ignore-change": "-b", "ignore-eol": "--ignore-space-at-eol", - "": ""} + "": "", + } return whitespaceFlags[whiteSpaceBehavior] } From 36e5828ffc6d881d91c0b798a614d87635c03b9a Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 14 Feb 2022 13:10:45 +0000 Subject: [PATCH 2/3] move close into the defer Signed-off-by: Andrew Thornton --- modules/git/repo_attribute.go | 13 +++++++------ modules/git/repo_language_stats_nogogit.go | 5 ++++- services/gitdiff/gitdiff.go | 1 - 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/modules/git/repo_attribute.go b/modules/git/repo_attribute.go index 0bb550bb4b89a..2ba16959bc547 100644 --- a/modules/git/repo_attribute.go +++ b/modules/git/repo_attribute.go @@ -87,7 +87,7 @@ func (repo *Repository) CheckAttribute(opts CheckAttributeOpts) (map[string]map[ return nil, fmt.Errorf("wrong number of fields in return from check-attr") } - var name2attribute2info = make(map[string]map[string]string) + name2attribute2info := make(map[string]map[string]string) for i := 0; i < (len(fields) / 3); i++ { filename := string(fields[3*i]) @@ -178,12 +178,13 @@ func (c *CheckAttributeReader) Init(ctx context.Context) error { // Run run cmd func (c *CheckAttributeReader) Run() error { - defer func() { - _ = c.Close() - }() stdErr := new(bytes.Buffer) err := c.cmd.RunInDirTimeoutEnvFullPipelineFunc(c.env, -1, c.Repo.Path, c.stdOut, stdErr, c.stdinReader, func(_ context.Context, _ context.CancelFunc) error { - close(c.running) + select { + case <-c.running: + default: + close(c.running) + } return nil }) if err != nil && c.ctx.Err() != nil && err.Error() != "signal: killed" { @@ -229,10 +230,10 @@ func (c *CheckAttributeReader) CheckPath(path string) (rs map[string]string, err // Close close pip after use func (c *CheckAttributeReader) Close() error { + c.cancel() err := c.stdinWriter.Close() _ = c.stdinReader.Close() _ = c.stdOut.Close() - c.cancel() select { case <-c.running: default: diff --git a/modules/git/repo_language_stats_nogogit.go b/modules/git/repo_language_stats_nogogit.go index 0b21bf6344c1c..adb11dd8fa6a0 100644 --- a/modules/git/repo_language_stats_nogogit.go +++ b/modules/git/repo_language_stats_nogogit.go @@ -88,7 +88,10 @@ func (repo *Repository) GetLanguageStats(commitID string) (map[string]int64, err } }() } - defer cancel() + defer func() { + _ = checker.Close() + cancel() + }() } } diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index babe7be77a5fd..28c034fafdcd2 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -1417,7 +1417,6 @@ func GetDiff(gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff if err != nil && err != ctx.Err() { log.Error("Unable to open checker for %s. Error: %v", opts.AfterCommitID, err) } - _ = checker.Close() cancel() }() } From 4db28fce1ad782e7b6d929e0918ef616ba7b6925 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Mon, 14 Feb 2022 16:04:30 +0000 Subject: [PATCH 3/3] lets try again Signed-off-by: Andrew Thornton --- modules/git/repo_attribute.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/modules/git/repo_attribute.go b/modules/git/repo_attribute.go index 2ba16959bc547..3f6f8943a9d4c 100644 --- a/modules/git/repo_attribute.go +++ b/modules/git/repo_attribute.go @@ -178,6 +178,10 @@ func (c *CheckAttributeReader) Init(ctx context.Context) error { // Run run cmd func (c *CheckAttributeReader) Run() error { + defer func() { + _ = c.stdinReader.Close() + _ = c.stdOut.Close() + }() stdErr := new(bytes.Buffer) err := c.cmd.RunInDirTimeoutEnvFullPipelineFunc(c.env, -1, c.Repo.Path, c.stdOut, stdErr, c.stdinReader, func(_ context.Context, _ context.CancelFunc) error { select { @@ -190,7 +194,6 @@ func (c *CheckAttributeReader) Run() error { if err != nil && c.ctx.Err() != nil && err.Error() != "signal: killed" { return fmt.Errorf("failed to run attr-check. Error: %w\nStderr: %s", err, stdErr.String()) } - return nil } @@ -232,8 +235,6 @@ func (c *CheckAttributeReader) CheckPath(path string) (rs map[string]string, err func (c *CheckAttributeReader) Close() error { c.cancel() err := c.stdinWriter.Close() - _ = c.stdinReader.Close() - _ = c.stdOut.Close() select { case <-c.running: default: