-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
Add a timeout to createBlameReader #11727
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The timeout duration is arbitrary. The value needs to be longer than any actual use, and should only kick in when processes aren't reaped manually for some reason. Closes go-gitea#11716
@techknowlogick thanks for the typo correction. Nice catch. |
ctx, cancel := context.WithCancel(DefaultContext) | ||
// This timeout was arbitrarily chosen just so that there are not hundreds of `git blame` | ||
// processes sticking around after some operations. | ||
ctx, cancel := context.WithTimeout(DefaultContext, 5*time.Minute) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use git command timeout setting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if that makes sense? Semantically they're rather different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean from these values: https://github.com/go-gitea/gitea/blob/master/custom/conf/app.example.ini#L910
Maybe new setting should be added for this
@mqudsi I'd like to take credit for it, but our CI system checks for typos and provides the correct spelling. If it hadn't done that I wouldn't have noticed :) |
Why not just use the requesting context as the parent context - then we don't need a timeout - when the request is cancelled the command should be cancelled. diff --git a/modules/git/blame.go b/modules/git/blame.go
index 5a9ae9a74..95ea29536 100644
--- a/modules/git/blame.go
+++ b/modules/git/blame.go
@@ -89,19 +89,19 @@ func (r *BlameReader) Close() error {
}
// 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)
if err != nil {
return nil, err
}
gitRepo.Close()
- return createBlameReader(repoPath, GitExecutable, "blame", commitID, "--porcelain", "--", file)
+ return createBlameReader(ctx, repoPath, GitExecutable, "blame", commitID, "--porcelain", "--", f>
}
-func createBlameReader(dir string, command ...string) (*BlameReader, error) {
+func createBlameReader(ctx context.Context, dir string, command ...string) (*BlameReader, error) {
// FIXME: graceful: This should have a timeout
- ctx, cancel := context.WithCancel(DefaultContext)
+ ctx, cancel := context.WithCancel(ctx)
cmd := exec.CommandContext(ctx, command[0], command[1:]...)
cmd.Dir = dir
cmd.Stderr = os.Stderr
diff --git a/routers/repo/blame.go b/routers/repo/blame.go
index d353f3b5d..51e74006e 100644
--- a/routers/repo/blame.go
+++ b/routers/repo/blame.go
@@ -127,7 +127,7 @@ func RefBlame(ctx *context.Context) {
return
}
- blameReader, err := git.CreateBlameReader(models.RepoPath(userName, repoName), commitID, fileNam>
+ blameReader, err := git.CreateBlameReader(ctx.Req.Context(), models.RepoPath(userName, repoName)>
if err != nil {
ctx.NotFound("CreateBlameReader", err)
return |
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 go-gitea#11716 Closes go-gitea#11727 Signed-off-by: Andrew Thornton <[email protected]>
#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 <[email protected]>
Backport go-gitea#12102 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 go-gitea#11716 Closes go-gitea#11727 Signed-off-by: Andrew Thornton <[email protected]>
Backport #12102 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 <[email protected]>
go-gitea#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 go-gitea#11716 Closes go-gitea#11727 Signed-off-by: Andrew Thornton <[email protected]>
The timeout duration is arbitrary. The value needs to be longer than any
actual use, and should only kick in when processes aren't reaped
manually for some reason.
Closes #11716