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

os: Remove and RemoveAll can't remove the current directory on AIX #32180

Open
Helflym opened this issue May 22, 2019 · 5 comments

Comments

Projects
None yet
4 participants
@Helflym
Copy link
Contributor

commented May 22, 2019

On AIX, unlinkat or rmdir syscall doesn't work on the CWD. Therefore, a program like the following one doesn't remove GOPATH.

$ cat removeall.go
package main

import (
	"io/ioutil"
	"log"
	"os"
)

func main() {
	GOPATH, err := ioutil.TempDir("", "TestRemoveAll")
	if err != nil {
		log.Panic(err)
	}

	if err := os.Chdir(GOPATH); err != nil {
		log.Panic(err)
	}
	if err := os.Remove(GOPATH); err != nil {
		log.Panic(err)
	}
}
$ ./removeall.go
2019/05/22 09:29:19 remove /tmp/TestRemoveAll186232865: device busy
panic: remove /tmp/TestRemoveAll186232865: device busy

goroutine 1 [running]:
log.Panic(0xa00010000088f58, 0x1, 0x1)
        /opt/freeware/src/packages/BUILD/go-root/src/log/log.go:338 +0x9c
main.main()
        /opt/freeware/src/packages/BUILD/go-root/own_test/goprogs/removeall.go:19 +0x138
exit status 2

There is no real documentation on that. However, rmdir seems to return EBUSY and unlinkat EPERM. So, we should be able to detect if os.Remove is failing because of that. However, what's the correct behavior if we want to fix that ?
Do we chdir to the parentdir ? "/tmp" ? the executable folder ? anything else ?
Or do we want to keep this behavior ?

Note that, when calling os.Removeall, everything except the directory itself is still being removed. Therefore, only a empty directory remains. But, as most of the tests are doing a defer os.Removeall, /tmp is filled with empty directories...

Do some other OSes have the same problem ?

Edit: POSIX rmdir doesn't specify which behavior must be done (cf https://pubs.opengroup.org/onlinepubs/9699919799/functions/rmdir.html)

If the directory is the root directory or the current working directory of any process, it is unspecified whether the function succeeds, or whether it shall fail and set errno to [EBUSY].

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

It's a shame that POSIX didn't define that, since as far as I know all other Unix systems do support removing the current directory. I don't think there is anything we can do here. We can't chdir in the general case since that will unexpectedly affect the other threads in the process.

Which tests are leaving behind temporary directories? It looks like the test helper function chtmpdir, in os_test.go, should remove the temporary directory on AIX, so maybe we can adjust those tests to use chtmpdir.

@Helflym

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

I don't have a precise list at the moment but it doesn't only deal with tests. After running ./all.bash,
I have the following folders remaining:
cgolife958293113:
cgostdio550024417:
go-build246746664:
go-build331120624:
go-build356351149:
go-build716678653:
go-build756358968:

cgolife and cgostdio are two tests under there are calling defer os.Removeall. We should be able to move back to the launching folder after the test without impacting other threads, I guess.
However, I have no idea where go-build folders come from.

@kardianos

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

FYI: this, or something similar to this was also a problem with windows. As far as I know, windows also can't remove a dir with a lock (including CWD). I haven't ran a builder in a while, but you may want to look for some windows specific code.

@bcmills bcmills added this to the Go1.14 milestone May 22, 2019

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

As you say, the cgolife directory is from misc/cgo/life/life_test.go. The cgostdio directory is from misc/cgo/stdio/stdio_test.go. It should be easy to fix those tests.

The go-build directories are more likely created by the go tool in (*Builder).Init in cmd/go/internal/work/action.go. But if the directories are indeed empty it's hard to know what left them. I suggest temporarily modifying the deferred function in (*Builder).Init to change the go: failed to remove work dir case to call os.Exit(1). That should cause the tests leaving them behind to fail, so we can see what is doing that.

@Helflym

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

FYI: this, or something similar to this was also a problem with windows. As far as I know, windows also can't remove a dir with a lock (including CWD). I haven't ran a builder in a while, but you may want to look for some windows specific code.

There is nothing obvious in os/file_windows.go. Maybe this is done on a case-by-case basis.

The go-build directories are more likely created by the go tool in (*Builder).Init in cmd/go/internal/work/action.go. But if the directories are indeed empty it's hard to know what left them. I suggest temporarily modifying the deferred function in (*Builder).Init to change the go: failed to remove work dir case to call os.Exit(1). That should cause the tests leaving them behind to fail, so we can see what is doing that.

I've tried but nothing occurred... It seems random once again. I'll update misc/cgo tests but will need to investigate more about those go-build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.