-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Prevent dangling cat-file calls (goroutine alternative) #19454
Prevent dangling cat-file calls (goroutine alternative) #19454
Conversation
If an `os/exec.Command` is passed non `*os.File` as an input/output, go will create `os.Pipe`s and wait for their closure in `cmd.Wait()`. If the code following this is responsible for closing `io.Pipe`s or other handlers then on process death from context cancellation the `Wait` can hang. There are two possible solutions: 1. use `os.Pipe` as the input/output as `cmd.Wait` does not wait for these. 2. create a goroutine waiting on the context cancellation that will close the inputs. This PR provides the second option - which is a simpler change that can be more easily backported. Closes go-gitea#19448 Signed-off-by: Andrew Thornton <art27@cantab.net>
I'm in favor of #19448 as it avoids the goroutine is more correct, however if that PR introduces performance problems we should revert back to this PR in which it still will use buffered I/O. |
I'd like to retrive some metrics ... - but we should backport THIS |
I vote we should do this and in the meantime I can work on #19448 and try to make it cleaner. |
OK ... one thing we might want to do which will prevent danglers for all calls to git: diff --git a/modules/git/command.go b/modules/git/command.go
index 3dd12e421..acc1ed66a 100644
--- a/modules/git/command.go
+++ b/modules/git/command.go
@@ -165,6 +165,30 @@ func (c *Command) Run(opts *RunOpts) error {
return err
}
+ closers := make([]io.Closer, 0, 3)
+ for _, pipe := range []interface{}{cmd.Stdout, cmd.Stdin, cmd.Stderr} {
+ if pipe == nil {
+ continue
+ }
+ if _, ok := pipe.(*os.File); ok {
+ continue
+ }
+
+ if closer, ok := pipe.(io.Closer); ok {
+ closers = append(closers, closer)
+ }
+ }
+
+ if len(closers) > 0 {
+ go func() {
+ <-ctx.Done()
+ cancel()
+ for _, closer := range closers {
+ _ = closer.Close()
+ }
+ }()
+ }
+
if opts.PipelineFunc != nil {
err := opts.PipelineFunc(ctx, cancel)
if err != nil { |
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.
ack for hotfix :)
If an `os/exec.Command` is passed non `*os.File` as an input/output, go will create `os.Pipe`s and wait for their closure in `cmd.Wait()`. If the code following this is responsible for closing `io.Pipe`s or other handlers then on process death from context cancellation the `Wait` can hang. There are two possible solutions: 1. use `os.Pipe` as the input/output as `cmd.Wait` does not wait for these. 2. create a goroutine waiting on the context cancellation that will close the inputs. This PR provides the second option - which is a simpler change that can be more easily backported. Closes go-gitea#19448 Signed-off-by: Andrew Thornton <art27@cantab.net>
-> #19466 |
) If an `os/exec.Command` is passed non `*os.File` as an input/output, go will create `os.Pipe`s and wait for their closure in `cmd.Wait()`. If the code following this is responsible for closing `io.Pipe`s or other handlers then on process death from context cancellation the `Wait` can hang. There are two possible solutions: 1. use `os.Pipe` as the input/output as `cmd.Wait` does not wait for these. 2. create a goroutine waiting on the context cancellation that will close the inputs. This PR provides the second option - which is a simpler change that can be more easily backported. Closes #19448 Signed-off-by: Andrew Thornton <art27@cantab.net> Co-authored-by: zeripath <art27@cantab.net>
* giteaofficial/main: [skip ci] Updated translations via Crowdin Mark TemplateLoading error as "UnprocessableEntity" (go-gitea#19445) Prevent dangling cat-file calls (goroutine alternative) (go-gitea#19454)
If an `os/exec.Command` is passed non `*os.File` as an input/output, go will create `os.Pipe`s and wait for their closure in `cmd.Wait()`. If the code following this is responsible for closing `io.Pipe`s or other handlers then on process death from context cancellation the `Wait` can hang. There are two possible solutions: 1. use `os.Pipe` as the input/output as `cmd.Wait` does not wait for these. 2. create a goroutine waiting on the context cancellation that will close the inputs. This PR provides the second option - which is a simpler change that can be more easily backported. Closes go-gitea#19448 Signed-off-by: Andrew Thornton <art27@cantab.net>
If an
os/exec.Command
is passed non*os.File
as an input/output, gowill create
os.Pipe
s and wait for their closure incmd.Wait()
. Ifthe code following this is responsible for closing
io.Pipe
s or otherhandlers then on process death from context cancellation the
Wait
canhang.
There are two possible solutions:
os.Pipe
as the input/output ascmd.Wait
does not wait for these.This PR provides the second option - which is a simpler change that can
be more easily backported.
Reference #19448
Fix #16113
Signed-off-by: Andrew Thornton art27@cantab.net