mirror of
https://github.com/go-gitea/gitea.git
synced 2026-10-05 02:41:47 +09:00
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>
215 lines
4.8 KiB
Go
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
|
|
}
|