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: IsNotExist returns false for syscall.ENOTDIR #18974

Closed
mastercactapus opened this issue Feb 7, 2017 · 8 comments
Closed

os: IsNotExist returns false for syscall.ENOTDIR #18974

mastercactapus opened this issue Feb 7, 2017 · 8 comments
Assignees
Milestone

Comments

@mastercactapus
Copy link
Contributor

@mastercactapus mastercactapus commented Feb 7, 2017

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

1.7.5 and 1.8rc3

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/nathan/projects"
GORACE=""
GOROOT="/Users/nathan/go"
GOTOOLDIR="/Users/nathan/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/mg/sm3pzxbx1n93_2jzksr2clwm0000gp/T/go-build206309359=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

Basic IsNotExist example: https://play.golang.org/p/mIvdT6IBHC
HTTP example: https://play.golang.org/p/y_wAm3nZpq

What did you expect to see?

  • os.IsNotExist(err) to be true if the file does not exist (and the error wasn't a network or other error)
  • a 404 from the fileserver in such cases

What did you see instead?

  • os.IsNotExist(err) was false
  • a 500 was returned from the fileserver

From the docs (and inferred from it's usage in stdlib)
"IsNotExist returns a boolean indicating whether the error is known to report that a file or directory does not exist. "

In this case, can we safely assume the "not a directory" error reports that the path does not exist?

@bradfitz bradfitz changed the title os.IsNotExist returns false if path contains a file ("is not a directory" error) os: IsNotExist returns false for syscall.ENOTDIR Feb 7, 2017
@bradfitz bradfitz added this to the Go1.9Maybe milestone Feb 7, 2017
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Feb 7, 2017

@ianlancetaylor, thoughts on whether syscall.ENOTDIR should be an os.IsNotExist?

@Ichbinjoe

This comment has been minimized.

Copy link

@Ichbinjoe Ichbinjoe commented Feb 7, 2017

@bradfitz on decision whether this should be handled, mind if I submit the code review? I am looking to get into contributing to go

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Feb 7, 2017

@Ichbinjoe, if you'd like, but it might be better to wait for a decision so you don't waste any time.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 7, 2017

Unfortunately I don't think that os.IsNotExist should return true for ENOTDIR. I agree that in your case it would make sense, but consider:

	f, _ := os.Open("/home/iant/hello.go")
	_, err := f.Readdirnames(0)

This will set err to syscall.ENOTDIR (wrapped in a os.SyscallError) but it would not make sense for os.IsNotExist to return true here. The file clearly exists. It's just not a directory. The docs for os.IsNotExist say that it returns "whether the error is known to report that a file or directory does not exist." In general, the ENOTDIR is not known to report that. It only sometimes does.

Closing.

@mastercactapus

This comment has been minimized.

Copy link
Contributor Author

@mastercactapus mastercactapus commented Feb 7, 2017

Ah, you're right. Context is important in this case, caller's of os.Open or similar would be able to derive that, but not for all cases with just the error.

It might be worthwhile to add something to the docs to clarify that the caller should take care to understand those implications. In file-access-type usages (like the http.FileServer) os.IsNotExist is not enough to determine if a not-found error should be returned -- but it seems to be common practice to do so.

I'll open a separate issue for the FileServer behavior.

@mastercactapus

This comment has been minimized.

Copy link
Contributor Author

@mastercactapus mastercactapus commented Feb 8, 2017

@ianlancetaylor I was working on a fix for the FileServer, and I came across some new light on this that I'd like to get your opinion on.

It would seem this behavior actually differs from platform to platform, and may be worth fixing in the os package after all.


With regards to "non-existant.file" and "non-existant.file/path":

On windows, ERROR_PATH_NOT_FOUND is returned instead of ERROR_FILE_NOT_FOUND (two separate errors), though both are reported as true from os.IsNotExist

But with the same code & file structure on a unix system, os.IsNotExist returns false in the second case (as per this issue report).

With the readdir example, I found that the errors are distinguishable too, so it should be solvable.

// os.Open
&os.PathError{Op:"open", Path:"test.go/foo", Err:0x14}

// fd.Readdir
&os.SyscallError{Syscall:"readdirent", Err:0x14}

If the os package:
"provides a platform-independent interface to operating system functionality"

And os.IsNotExist checks:
"whether the error is known to report that a file or directory does not exist."

I think it would be valid to consider resolving this within the os package. A precise fix for this in net/http (or other packages) would require build flags (syscall references), which feels like something that should stay in os if possible.

Let me know what your thoughts are on this.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 8, 2017

It's impossible for the os package to fully handle all cases identically across operating systems. I don't think we want to add the complexity of having os.IsNotExist try to second guess what the program was doing.

For example, I can get the exact error you show by calling os.OpenFile("foo.go", syscall.O_DIRECTORY, 0). It's an odd thing to do, to be sure, but again it's a file that exists even though I get back &os.PathError{Op:"open", Path:"foo.go", Err:0x14}.

@mastercactapus

This comment has been minimized.

Copy link
Contributor Author

@mastercactapus mastercactapus commented Feb 8, 2017

Darn, I hadn't considered that. Thank you for taking the time to comment back on this, your insight was very helpful!

@golang golang locked and limited conversation to collaborators Feb 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.