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: consider syscall.EEXIST in os.IsExist #29295

Open
bep opened this issue Dec 16, 2018 · 11 comments

Comments

@bep
Copy link
Contributor

@bep bep commented Dec 16, 2018

Note that I'm not very categorical in my issue title, but this debugging of a subtle Afero file system bug has given me enough gray hairs to at least deserve an issue/discussion.

The program below runs fine on *nix but fails on Windows (it compiles fine):

package main

import (
	"log"
	"os"
	"syscall"
)

func main() {
	if !os.IsExist(syscall.EEXIST) {
		log.Fatal("failed")
	}
}

I assume that syscall.EEXIST will never happen on Windows, and then it should probably also not be defined for Windows. I guess that ship has sailed, but you should then consider to expand os.IsExist on Windows to also include this error. Because there may be other people creating file system abstractions that can be bitten by this.

/cc @spf13

@ianlancetaylor ianlancetaylor changed the title os: Consider syscall.EEXIST in os.IsExist os: consider syscall.EEXIST in os.IsExist Dec 17, 2018
@ianlancetaylor ianlancetaylor added this to the Go1.13 milestone Dec 17, 2018
@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Dec 17, 2018

I assume that syscall.EEXIST will never happen on Windows,

syscall.EEXIST will never happen on Windows,

and then it should probably also not be defined for Windows.

It should not be defined on Windows.

As far as I remember (I could be wrong), when first Windows version of Go appeared, we copied modified UNIX version of syscall package to make all other packages work. And only later we started writing Windows code in packages other than syscall.

I guess that ship has sailed,

Maybe we can remove these symbols in Go 2. Whatever Go 2 is.

but you should then consider to expand os.IsExist on Windows to also include this error. Because there may be other people creating file system abstractions that can be bitten by this.

I don't see syscall.EEXIST used under $GOROOT on Windows. I doubt anyone uses syscall.EEXIST on Windows. I think, expanding os.IsExist to include syscall.EEXIST will confuse this matter even more. I think we should leave it alone.

syscall package is platform specific. You cannot assume that syscall.EEXIST will behave one way or the other on Windows.

Alex

@bep

This comment has been minimized.

Copy link
Contributor Author

@bep bep commented Dec 17, 2018

I doubt anyone uses syscall.EEXIST on Windows.

I linked to a widely used example in my issue report:

spf13/afero@5e9f8ec

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Dec 17, 2018

I doubt anyone uses syscall.EEXIST on Windows.

I linked to a widely used example in my issue report:

Fair enough. I take my word about "anyone uses syscall.EEXIST on Windows". But my point still stands - you assumed things about syscall.EEXIST that were never there. You fixed your code. I don't think there is anything else to do.

Alex

@bep

This comment has been minimized.

Copy link
Contributor Author

@bep bep commented Dec 17, 2018

you assumed things about syscall.EEXIST that were never there.

Let me be clear: This is not my code. I fixed some other person's code. And the repository I linked to is owned by a person that happens to also be on the Go team at Google -- which may indicate that this is a mistake that is a little bit too easy to make.

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Dec 17, 2018

And the repository I linked to is owned by a person that happens to also be on the Go team at Google -- which may indicate that this is a mistake that is a little bit too easy to make.

Everyone makes mistakes. Only people who don't write code do not make mistakes.

The person who wrote the code, should not have used syscall.EEXIST on Windows. That is the mistake here as far as I am concerned. How do you propose to stop feature developers to use syscall.EEXIST on Windows by changing os.IsExist?

Alex

@networkimprov

This comment has been minimized.

Copy link

@networkimprov networkimprov commented Dec 17, 2018

@bep you could propose a documentation change to warn readers about this...

@spf13

This comment has been minimized.

Copy link
Contributor

@spf13 spf13 commented Dec 18, 2018

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 18, 2018

It doesn't seem so bad to me to have os.IsExist accept syscall.EEXIST on Windows. It may have been a mistake to define syscall.EEXIST at all on Windows. But once we made that mistake, it seems that we may as well carry it forward to functions like os.IsExist, for precisely this reason: because some code that runs on Windows may start using it.

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Dec 18, 2018

This sounds to me like a documentation change needs to be made.

Any documentation sounds good to me. I just do not know what to say - Do not use EEXIST!? It would require some explanation of why not to use.

But once we made that mistake, it seems that we may as well carry it forward to functions like os.IsExist, for precisely this reason: because some code that runs on Windows may start using it.

I do not see reason for syscall.EEXIST to be used on Windows. So I believe code with syscall.EEXIST is broken. I don't see how making os.IsExist(syscall.EEXIST) return true helps in this situation.

What would the code that uses syscall.EEXIST would look like?

Alex

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 18, 2018

The code that uses syscall.EEXIST would look like

// Special purpose file system layer available on both Unix and Windows.
func (mfs *MyFileSystem) Open(path string) (*MyFile, error) {
    if mfs.noSuchPath(path) {
        return nil, syscall.EEXIST // this error is available on both Unix and WIndows
    }
    ...
}
@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Dec 21, 2018

The code that uses syscall.EEXIST would look like

Sorry my message is long.

It is mistake (in my opinion) to import syscall package into the source file that is compiled for both Windows and Linux (I assumed your code builds on both *_linux.go and *_windows.go).

I know source files like that happens here and there, but I find they always causes problems for me.

For example, for #28477 (comment) I tried to make $GOROOT/os/pipe_test.go to work on Windows. So I removed !windows from +build line, and the pipe_test.go file "nearly" builds. pipe_test.go file uses syscall package in many places, but since it builds I decided to try and make it work. Unfortunately it is not easy - Linux and Windows versions are similar (they have similar symbols), but semantically everything works differently. Linux process uses 0, 1, 2 for stdin, stdout, stderr, but Windows do not. Linux can have extra file handles passed to the new process, but Windows cannot. It was very confusing and time consuming to debug.

Also, please show what mfs.noSuchPath does. If mfs.noSuchPath uses os package to implement its logic, then mfs.noSuchPath should just return whatever error it receives from os package. If mfs.noSuchPath does not uses os package, then you should not be using os.IsExist to test returned errors. Unless you write tests that show it is OK to use os.IsExist.

Also not many people will be writing code like yours. But os.IsExist code is read by everyone. If we are to make code change, we would need some good comment there explaining the problem and our reasoning.

Also, if we start worrying about people using syscall.EEXIST incorrectly only because EEXIST name exists on both Windows and Linux, we should worry about many other errors in syscall package: ENODEV, ENOEXEC and many others. Should we also try and make syscall.Syscall "safer" to use for people who just build their Linux code on Windows?

Alex

@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.