diff --git a/modules/git/command.go b/modules/git/command.go index 22cb275ab2..fd781ffef4 100644 --- a/modules/git/command.go +++ b/modules/git/command.go @@ -46,6 +46,7 @@ type Command struct { desc string globalArgsLength int brokenArgs []string + cmd *exec.Cmd // for debug purpose only } func (c *Command) String() string { @@ -314,6 +315,7 @@ func (c *Command) Run(opts *RunOpts) error { startTime := time.Now() cmd := exec.CommandContext(ctx, c.prog, c.args...) + c.cmd = cmd // for debug purpose only if opts.Env == nil { cmd.Env = os.Environ() } else { diff --git a/modules/git/repo_attribute.go b/modules/git/repo_attribute.go index 90eb783fe8..99073a8477 100644 --- a/modules/git/repo_attribute.go +++ b/modules/git/repo_attribute.go @@ -9,6 +9,8 @@ import ( "fmt" "io" "os" + "path/filepath" + "time" "code.gitea.io/gitea/modules/log" ) @@ -102,7 +104,7 @@ type CheckAttributeReader struct { stdinReader io.ReadCloser stdinWriter *os.File - stdOut attributeWriter + stdOut *nulSeparatedAttributeWriter cmd *Command env []string ctx context.Context @@ -152,7 +154,6 @@ func (c *CheckAttributeReader) Init(ctx context.Context) error { return nil } -// Run run cmd func (c *CheckAttributeReader) Run() error { defer func() { _ = c.stdinReader.Close() @@ -176,7 +177,7 @@ func (c *CheckAttributeReader) Run() error { func (c *CheckAttributeReader) CheckPath(path string) (rs map[string]string, err error) { defer func() { if err != nil && err != c.ctx.Err() { - log.Error("Unexpected error when checking path %s in %s. Error: %v", path, c.Repo.Path, err) + log.Error("Unexpected error when checking path %s in %s, error: %v", path, filepath.Base(c.Repo.Path), err) } }() @@ -191,9 +192,31 @@ func (c *CheckAttributeReader) CheckPath(path string) (rs map[string]string, err return nil, err } + reportTimeout := func() error { + stdOutClosed := false + select { + case <-c.stdOut.closed: + stdOutClosed = true + default: + } + debugMsg := fmt.Sprintf("check path %q in repo %q", path, filepath.Base(c.Repo.Path)) + debugMsg += fmt.Sprintf(", stdOut: tmp=%q, pos=%d, closed=%v", string(c.stdOut.tmp), c.stdOut.pos, stdOutClosed) + if c.cmd.cmd != nil { + debugMsg += fmt.Sprintf(", process state: %q", c.cmd.cmd.ProcessState.String()) + } + _ = c.Close() + return fmt.Errorf("CheckPath timeout: %s", debugMsg) + } + rs = make(map[string]string) for range c.Attributes { select { + case <-time.After(5 * time.Second): + // There is a strange "hang" problem in gitdiff.GetDiff -> CheckPath + // So add a timeout here to mitigate the problem, and output more logs for debug purpose + // In real world, if CheckPath runs long than seconds, it blocks the end user's operation, + // and at the moment the CheckPath result is not so important, so we can just ignore it. + return nil, reportTimeout() case attr, ok := <-c.stdOut.ReadAttribute(): if !ok { return nil, c.ctx.Err() @@ -206,18 +229,12 @@ func (c *CheckAttributeReader) CheckPath(path string) (rs map[string]string, err return rs, nil } -// Close close pip after use func (c *CheckAttributeReader) Close() error { c.cancel() err := c.stdinWriter.Close() return err } -type attributeWriter interface { - io.WriteCloser - ReadAttribute() <-chan attributeTriple -} - type attributeTriple struct { Filename string Attribute string @@ -281,7 +298,7 @@ func (wr *nulSeparatedAttributeWriter) Close() error { return nil } -// Create a check attribute reader for the current repository and provided commit ID +// CheckAttributeReader creates a check attribute reader for the current repository and provided commit ID func (repo *Repository) CheckAttributeReader(commitID string) (*CheckAttributeReader, context.CancelFunc) { indexFilename, worktree, deleteTemporaryFile, err := repo.ReadTreeToTemporaryIndex(commitID) if err != nil { @@ -303,21 +320,21 @@ func (repo *Repository) CheckAttributeReader(commitID string) (*CheckAttributeRe } ctx, cancel := context.WithCancel(repo.Ctx) if err := checker.Init(ctx); err != nil { - log.Error("Unable to open checker for %s. Error: %v", commitID, err) + log.Error("Unable to open attribute checker for commit %s, error: %v", commitID, err) } else { go func() { err := checker.Run() - if err != nil && err != ctx.Err() { - log.Error("Unable to open checker for %s. Error: %v", commitID, err) + if err != nil && !IsErrCanceledOrKilled(err) { + log.Error("Attribute checker for commit %s exits with error: %v", commitID, err) } cancel() }() } - deferable := func() { + deferrable := func() { _ = checker.Close() cancel() deleteTemporaryFile() } - return checker, deferable + return checker, deferrable } diff --git a/modules/git/repo_attribute_test.go b/modules/git/repo_attribute_test.go index 0fcd94b4c7..2e1abe17a9 100644 --- a/modules/git/repo_attribute_test.go +++ b/modules/git/repo_attribute_test.go @@ -4,10 +4,16 @@ package git import ( + "context" + mathRand "math/rand/v2" + "path/filepath" + "slices" + "sync" "testing" "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func Test_nulSeparatedAttributeWriter_ReadAttribute(t *testing.T) { @@ -95,3 +101,57 @@ func Test_nulSeparatedAttributeWriter_ReadAttribute(t *testing.T) { Value: "unspecified", }, attr) } + +func TestAttributeReader(t *testing.T) { + t.Skip() // for debug purpose only, do not run in CI + + ctx := context.Background() + + timeout := 1 * time.Second + repoPath := filepath.Join(testReposDir, "language_stats_repo") + commitRef := "HEAD" + + oneRound := func(t *testing.T, roundIdx int) { + ctx, cancel := context.WithTimeout(ctx, timeout) + _ = cancel + gitRepo, err := OpenRepository(ctx, repoPath) + require.NoError(t, err) + defer gitRepo.Close() + + commit, err := gitRepo.GetCommit(commitRef) + require.NoError(t, err) + + files, err := gitRepo.LsFiles() + require.NoError(t, err) + + randomFiles := slices.Clone(files) + randomFiles = append(randomFiles, "any-file-1", "any-file-2") + + t.Logf("Round %v with %d files", roundIdx, len(randomFiles)) + + attrReader, deferrable := gitRepo.CheckAttributeReader(commit.ID.String()) + defer deferrable() + + wg := sync.WaitGroup{} + wg.Add(1) + + go func() { + for { + file := randomFiles[mathRand.IntN(len(randomFiles))] + _, err := attrReader.CheckPath(file) + if err != nil { + for i := 0; i < 10; i++ { + _, _ = attrReader.CheckPath(file) + } + break + } + } + wg.Done() + }() + wg.Wait() + } + + for i := 0; i < 100; i++ { + oneRound(t, i) + } +} diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index 8d6e9f38e0..6f7890c448 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -1193,6 +1193,8 @@ func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, fi if language.Has() { diffFile.Language = language.Value() } + } else { + checker = nil // CheckPath fails, it's not impossible to "check" anymore } }