Files
565957503c fix(git): avoid unnecessary timers during language stats (#39531) (#39535)
Backport #39531 by @CalvinTjoaquinn

## What

`BatchChecker.CheckPath` uses `time.After` inside its read loop. This
replaces it with a `time.Timer` that is stopped once the attribute
arrives.

## Why

```go
for i := 0; i < c.attributesNum; i++ {
	select {
	case <-time.After(5 * time.Second):
		// there is no "hang" problem now. This code is just used to catch other potential problems.
		return nil, reportTimeout()
	case attr, ok := <-c.stdOut.ReadAttribute():
```

`time.After` has no way to be cancelled, so the timer it allocates stays
in the runtime timer heap for the full five seconds whichever case the
`select` picks. On the normal path the attribute arrives immediately and
the timer is abandoned while still pending.

The multiplier is what makes it worth changing rather than leaving as
noise. The loop runs `len(LinguistAttributes)` times, which is six, and
`CheckPath` is called once per file:

```go
// modules/git/languagestats/language_stats_get.go:95, in the loop over repository files
attrs, err := checker.CheckPath(f.Name())
// services/gitdiff/gitdiff.go:1408, in the loop over diff files
attrs, err := checker.CheckPath(diffFile.Name)
```

So a language-stats pass over a repository of N files holds up to 6N
pending five-second timers, and a large diff does the same per file. By
the comment's own account that timeout path does not fire in practice,
so every one of those timers is allocated and held for nothing.

## The change

`time.NewTimer` plus `Stop` on the paths that win, which keeps the
behaviour identical: each iteration still gets its own five-second
budget, and the timer is released as soon as the attribute or the
context arrives rather than five seconds later.

If you would rather have a single budget for the whole read, one timer
hoisted above the loop with `defer timeout.Stop()` is simpler and
stricter, since six attributes from an already running `git check-attr`
should arrive together. That changes the semantics from per-attribute to
per-call, so I left it alone and am happy to switch if you prefer it.

## Verification

```
go build ./modules/git/...
go vet ./modules/git/attribute/
go test -count=1 ./modules/git/attribute/      # 11 tests, 0 failures
golangci-lint run ./modules/git/attribute/...
gofmt -l modules/git/attribute/                # no output
```

The package's own `CheckPath` tests cover the success path, the
closed-stdout path and the context-cancelled path, which are the three
`select` arms touched here.

Found with a small AST pass over the tree looking for `time.After`
inside loop bodies. `staticcheck`'s SA1015 covers `time.Tick` and says
nothing about this shape, so no linter in the current set reports it. Of
the nine other hits in the tree the rest look deliberate or harmless,
and `modules/queue/workergroup.go` already guards against exactly this
by only creating a debounce timer when none is pending, so I only
changed this one.

<sub>Disclosure per the AI Contribution Policy: I used an AI tool to
help find this and to draft the description. The counts above are
`len(LinguistAttributes)` and the two call sites cited, so they can be
checked directly.</sub>

Signed-off-by: Calvin Tjoaquinn <calvintjoa23@gmail.com>
Co-authored-by: Calvin Tjoaquinn <66313400+CalvinTjoaquinn@users.noreply.github.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
2026-10-01 19:44:48 +00:00

215 lines
4.8 KiB
Go

// Copyright 2019 The Gitea Authors. All rights reserved.
// SPDX-License-Identifier: MIT
package attribute
import (
"bytes"
"context"
"fmt"
"io"
"time"
"gitea.dev/modules/git"
"gitea.dev/modules/git/gitcmd"
"gitea.dev/modules/log"
"gitea.dev/modules/setting"
)
// BatchChecker provides a reader for check-attribute content that can be long running
type BatchChecker struct {
attributesNum int
repo *git.Repository
stdinWriter io.WriteCloser
stdOut *nulSeparatedAttributeWriter
ctx context.Context
cancel context.CancelFunc
cmd *gitcmd.Command
}
// NewBatchChecker creates a check attribute reader for the current repository and provided commit ID
// If treeish is empty, then it will use current working directory, otherwise it will use the provided treeish on the bare repo
func NewBatchChecker(ctx context.Context, repo *git.Repository, treeish string, attributes []string) (checker *BatchChecker, returnedErr error) {
ctx, cancel := context.WithCancel(ctx)
defer func() {
if returnedErr != nil {
cancel()
}
}()
cmd, envs, cleanup, err := checkAttrCommand(ctx, repo, treeish, nil, attributes)
if err != nil {
return nil, err
}
defer func() {
if returnedErr != nil {
cleanup()
}
}()
cmd.AddArguments("--stdin")
checker = &BatchChecker{
attributesNum: len(attributes),
repo: repo,
ctx: ctx,
cmd: cmd,
cancel: func() {
cancel()
cleanup()
},
}
stdinWriter, stdinWriterClose := cmd.MakeStdinPipe()
checker.stdinWriter = stdinWriter
lw := new(nulSeparatedAttributeWriter)
lw.attributes = make(chan attributeTriple, len(attributes))
lw.closed = make(chan struct{})
checker.stdOut = lw
cmd.WithEnv(envs).
WithRepo(repo).
WithStdoutCopy(lw)
go func() {
defer stdinWriterClose()
defer checker.cancel()
defer lw.Close()
err := cmd.RunWithStderr(ctx)
if err != nil && !gitcmd.IsErrorCanceledOrKilled(err) {
log.Error("Attribute checker for commit %s exits with error: %v", treeish, err)
}
}()
return checker, nil
}
// CheckPath check attr for given path
func (c *BatchChecker) CheckPath(path string) (rs *Attributes, 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.LogString(), err)
}
}()
select {
case <-c.ctx.Done():
return nil, c.ctx.Err()
default:
}
if _, err = c.stdinWriter.Write([]byte(path + "\x00")); err != nil {
defer c.Close()
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, c.repo.LogString())
debugMsg += fmt.Sprintf(", stdOut: tmp=%q, pos=%d, closed=%v", string(c.stdOut.tmp), c.stdOut.pos, stdOutClosed)
if c.cmd != nil {
debugMsg += fmt.Sprintf(", process state: %q", c.cmd.ProcessState())
}
_ = c.Close()
return fmt.Errorf("CheckPath timeout: %s", debugMsg)
}
timeout := time.NewTimer(5 * time.Second)
defer timeout.Stop()
rs = NewAttributes()
for i := 0; i < c.attributesNum; i++ {
select {
case <-timeout.C:
// there is no "hang" problem now. This code is just used to catch other potential problems.
err = reportTimeout()
setting.PanicInDevOrTesting("Unexpected timeout, need to investigate: %v", err)
return nil, err
case attr, ok := <-c.stdOut.ReadAttribute():
if !ok {
return nil, c.ctx.Err()
}
rs.m[attr.Attribute] = Attribute(attr.Value)
case <-c.ctx.Done():
return nil, c.ctx.Err()
}
}
return rs, nil
}
func (c *BatchChecker) Close() error {
c.cancel()
err := c.stdinWriter.Close()
return err
}
type attributeTriple struct {
Filename string
Attribute string
Value string
}
type nulSeparatedAttributeWriter struct {
tmp []byte
attributes chan attributeTriple
closed chan struct{}
working attributeTriple
pos int
}
func (wr *nulSeparatedAttributeWriter) Write(p []byte) (n int, err error) {
l, read := len(p), 0
nulIdx := bytes.IndexByte(p, '\x00')
for nulIdx >= 0 {
wr.tmp = append(wr.tmp, p[:nulIdx]...)
switch wr.pos {
case 0:
wr.working = attributeTriple{
Filename: string(wr.tmp),
}
case 1:
wr.working.Attribute = string(wr.tmp)
case 2:
wr.working.Value = string(wr.tmp)
}
wr.tmp = wr.tmp[:0]
wr.pos++
if wr.pos > 2 {
wr.attributes <- wr.working
wr.pos = 0
}
read += nulIdx + 1
if l > read {
p = p[nulIdx+1:]
nulIdx = bytes.IndexByte(p, '\x00')
} else {
return l, nil
}
}
wr.tmp = append(wr.tmp, p...)
return l, nil
}
func (wr *nulSeparatedAttributeWriter) ReadAttribute() <-chan attributeTriple {
return wr.attributes
}
func (wr *nulSeparatedAttributeWriter) Close() error {
select {
case <-wr.closed:
return nil
default:
}
close(wr.attributes)
close(wr.closed)
return nil
}