Skip to content
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

Merged
merged 4 commits into from
Apr 22, 2022

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Apr 21, 2022

If an os/exec.Command is passed non *os.File as an input/output, go
will create os.Pipes and wait for their closure in cmd.Wait(). If
the code following this is responsible for closing io.Pipes 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.

Reference #19448
Fix #16113

Signed-off-by: Andrew Thornton art27@cantab.net

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>
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Apr 21, 2022
@Gusted
Copy link
Contributor

Gusted commented Apr 21, 2022

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.

@6543
Copy link
Member

6543 commented Apr 21, 2022

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

@zeripath zeripath mentioned this pull request Apr 21, 2022
4 tasks
@zeripath
Copy link
Contributor Author

I vote we should do this and in the meantime I can work on #19448 and try to make it cleaner.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 21, 2022
@zeripath
Copy link
Contributor Author

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 {

Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack for hotfix :)

@6543 6543 merged commit 0dcc74a into go-gitea:main Apr 22, 2022
6543 pushed a commit to 6543-forks/gitea that referenced this pull request Apr 22, 2022
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>
@6543
Copy link
Member

6543 commented Apr 22, 2022

-> #19466

@6543 6543 added the backport/done All backports for this PR have been created label Apr 22, 2022
zeripath added a commit that referenced this pull request Apr 22, 2022
)

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>
@zeripath zeripath deleted the fix-dangling-cat-file-again-alternative branch April 22, 2022 15:59
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 23, 2022
* 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)
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
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>
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Task running since 2 weeks
5 participants