Ignore Sync errors on pipes when doing CheckAttributeReader.CheckPath, fix the hang of git cat-file

[wxiaoguang]

It is not necessary to call Sync on pipes. And the Sync usually results in error: "sync |1: invalid argument".

Ref: https://man7.org/linux/man-pages/man2/fdatasync.2.html :

EROFS, EINVAL:  fd is bound to a special file (e.g., a pipe, FIFO, or socket) which does not support synchronization.

If we return early from CheckPath when Sync returns error, the GetLanguageStats will hang forever (because the stdin's buffer will be fully filled, then Write in CheckPath blocks)

But I can not confirm the Sync behaviors on different platforms, so I think it's OK to keep it while ignore the returned error.

Related issues:

Bug report:

• CheckAttributeReader.CheckPath hangs forever, and make the background process never end #17094

Add caller to cat-file batch calls for debug:

• Add caller to cat-file batch calls (#17082) #17089
• Add caller to cat-file batch calls #17082

[zeripath]

This was my worry when we added the error check here in the first place.

But I couldn't force it to happen myself - Did you actually get this error reported? If you didn't - it's possible something else is actually happening:

I added the Sync because I was also seeing an unusual hold-up likely because the \x00 is not recognised as a line and because the buffer on a pipe can be up to 64Kb. The Sync appeared to cause a flush in my testing so the unexpected hold-up disappeared.

Now if you're experiencing some block then the Sync may not be enough to solve the problem - especially if you're not actually seeing the above error that you were expecting.

[zeripath]

Looking again at the code - I think you're likely suffering from the problem I was experiencing earlier.

Can you repeatedly replicate this issue?

[zeripath]

diff --git a/modules/git/repo_attribute.go b/modules/git/repo_attribute.go
index 0bd7d7e49..f001e65e3 100644
--- a/modules/git/repo_attribute.go
+++ b/modules/git/repo_attribute.go
@@ -112,13 +112,15 @@ func (c *CheckAttributeReader) Init(ctx context.Context) error {
 
 	if len(c.IndexFile) > 0 && CheckGitVersionAtLeast("1.7.8") == nil {
 		cmdArgs = append(cmdArgs, "--cached")
-		c.env = []string{"GIT_INDEX_FILE=" + c.IndexFile}
+		c.env = append(c.env, "GIT_INDEX_FILE="+c.IndexFile)
 	}
 
 	if len(c.WorkTree) > 0 && CheckGitVersionAtLeast("1.7.8") == nil {
-		c.env = []string{"GIT_WORK_TREE=" + c.WorkTree}
+		c.env = append(c.env, "GIT_WORK_TREE="+c.WorkTree)
 	}
 
+	c.env = append(c.env, "GIT_FLUSH=1")
+
 	if len(c.Attributes) > 0 {
 		cmdArgs = append(cmdArgs, c.Attributes...)
 		cmdArgs = append(cmdArgs, "--")

Could you check if this helps?

[wxiaoguang]

Your patch is good, but it doesn't help about this problem. They are different problems.

Linux document has said that a pipe should NOT be fsync-ed ( https://man7.org/linux/man-pages/man2/fdatasync.2.html )

So I think my patch makes the problem better than before.

On you side, the Sync is still been called, so you do not worry about buffered data.

On my side, the Sync error is ignored, the whole logic is correct.

If one day we have enough confidence about the pipe's behavior on all platforms, we can optimize this logic again.

ps: I have written a test program to test Sync, the result is:

2021/09/20 12:44:32 GOOS=darwin
2021/09/20 12:44:32 pipeWriter.Sync error: sync |1: bad file descriptor

2021/09/20 12:52:25 GOOS=linux
2021/09/20 12:52:25 pipeWriter.Sync error: sync |1: invalid argument

It's obviously wrong to call Sync on pipes in Linux / macOS.

Test Program:

package main

import (
	"log"
	"os"
	"os/exec"
	"runtime"
)

func main() {
	log.Printf("GOOS=%v\n", runtime.GOOS)

	var err error
	pipeReader, pipeWriter, err := os.Pipe()
	if err != nil {
		log.Fatalf("os.Pipe error: %v", err)
	}

	cmd := exec.Command("/bin/cat")
	cmd.Stdin = pipeReader
	err = cmd.Start()
	if err != nil {
		log.Fatalf("cmd.Start error: %v", err)
	}

	_, err = pipeWriter.WriteString("test")
	if err != nil {
		log.Fatalf("pipeWriter.Write error: %v", err)
	}

	err = pipeWriter.Sync()
	if err != nil {
		log.Fatalf("pipeWriter.Sync error: %v", err)
	}
}

[wxiaoguang]

Did you actually get this error reported? If you didn't - it's possible something else is actually happening:
Can you repeatedly replicate this issue?

Both yes, I added many debug logs and I am sure that ignore the Sync error will make the logic correct, and CheckPath can successfully ReadAttribute without Sync on my side.

And you can refer the test program to test Sync on pipes.

[zeripath]

I too am running on Linux but didn't get the sync error you do - otherwise I'd have never put it in. I even tested in a Windows virtual machine.

Hmm, are you building with go 1.17? I know that at some point go moved to use pipe2 - maybe that doesn't error on sync. Or maybe it's a kernel difference.

---

I also don't understand how an err at the Sync causes a hang. The logic there should abort and cancel the reader before a hang was possible at Read - at least that was how I intended it - so if there's a hang despite the error then that logic is failing somehow.

So I'm doubly confused and that makes me concerned that I've got the channel logic wrong somewhere in there - but I just can't see it.

---

Anyway if GIT_FLUSH env reliably prevents this hang along with ignoring the error on sync then I am happy.

But if the actual slow down/hang is not occurring because of the sync but at read - then you're likely suffering the same issue that the original native git implementation of commit info was suffering. That is, that although the write has been sent it's not reaching the git program or the git program's output is not reaching the read. If GIT_FLUSH prevents this then that was due to git buffering, if not then...

Perhaps placing a runtime.Gosched() in between the write and the read is needed? Another is changing the code to make things non-synchronous - that is push all of the paths in to git attribute channel, closing the writer at the end and reading them all. This would both fill the buffer and force go to schedule the other goroutines.

[wxiaoguang]

About environment

I have tested the Sync on pipes with:

• Debian 10 Linux 4.19.0-16-amd64 SMP Debian 4.19.181-1 (2021-03-19) x86_64 GNU/Linux
  go version go1.16.2 linux/amd64
• Debian 11 Linux 5.10.0-8-amd64 SMP Debian 5.10.46-4 (2021-08-03) x86_64 GNU/Linux
  go version go1.17 darwin/amd64 cross-compile with GOOS=linux
• macOS 11.5.2
  go version go1.17 darwin/amd64

Sync on pipes always fails and reports error sync |1: bad file descriptor for macOS or sync |1: invalid argument for Linux.

I believe that since Linux document says that fsync on pipes will fail, then here is a problem.

Why there is a hang

It seems that there are other flaws when running git cat-file or git check-attr, eg: when the git process is killed by timeout, the go routine still exists, then I still can see a background git process has been running for over 3 hours (in the process.Manager), but I can not find the process in system's process list by ps.

Because there may be unknown bugs, when Sync returns an error and CheckPath returns early, there are more calls to CheckPath and continue to fill the stdin buffer, then the Write in CheckPath blocks.