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

proposal: syscall: define Windows O_ALLOW_DELETE for use in os.OpenFile #34681

Open
rsc opened this issue Oct 3, 2019 · 54 comments
Labels
Projects
Milestone

Comments

@rsc
Copy link
Contributor

@rsc rsc commented Oct 3, 2019

The discussion on #32088 has so far been unable to reach consensus on setting FILE_SHARE_DELETE unconditionally in os.OpenFile. Right now it looks likely to be declined, possibly revisited in a few years.

In the interim, it is more difficult on Windows than on Unix to pass an extra flag to open a file with package syscall. syscall.Open takes fake O_* flags, not actual Windows permission bits. It wraps syscall.CreateFile, but it does a lot of argument preparation before making that call:

// package syscall

func Open(path string, mode int, perm uint32) (fd Handle, err error) {
	if len(path) == 0 {
		return InvalidHandle, ERROR_FILE_NOT_FOUND
	}
	pathp, err := UTF16PtrFromString(path)
	if err != nil {
		return InvalidHandle, err
	}
	var access uint32
	switch mode & (O_RDONLY | O_WRONLY | O_RDWR) {
	case O_RDONLY:
		access = GENERIC_READ
	case O_WRONLY:
		access = GENERIC_WRITE
	case O_RDWR:
		access = GENERIC_READ | GENERIC_WRITE
	}
	if mode&O_CREAT != 0 {
		access |= GENERIC_WRITE
	}
	if mode&O_APPEND != 0 {
		access &^= GENERIC_WRITE
		access |= FILE_APPEND_DATA
	}
	sharemode := uint32(FILE_SHARE_READ | FILE_SHARE_WRITE)
	var sa *SecurityAttributes
	if mode&O_CLOEXEC == 0 {
		sa = makeInheritSa()
	}
	var createmode uint32
	switch {
	case mode&(O_CREAT|O_EXCL) == (O_CREAT | O_EXCL):
		createmode = CREATE_NEW
	case mode&(O_CREAT|O_TRUNC) == (O_CREAT | O_TRUNC):
		createmode = CREATE_ALWAYS
	case mode&O_CREAT == O_CREAT:
		createmode = OPEN_ALWAYS
	case mode&O_TRUNC == O_TRUNC:
		createmode = TRUNCATE_EXISTING
	default:
		createmode = OPEN_EXISTING
	}
	h, e := CreateFile(pathp, access, sharemode, sa, createmode, FILE_ATTRIBUTE_NORMAL, 0)
	return h, e
}

A direct call to syscall.CreateFile would have to duplicate all that code.

A direct call to syscall.Open would still lose out on the adjustments made inside os.Open, most importantly the call to fixLongPath:

// package os

func openFile(name string, flag int, perm FileMode) (file *File, err error) {
	r, e := syscall.Open(fixLongPath(name), flag|syscall.O_CLOEXEC, syscallMode(perm))
	if e != nil {
		return nil, e
	}
	return newFile(r, name, "file"), nil
}

If package syscall on Windows defined a bit O_ALLOW_DELETE (maybe 0x100000), then syscall.Open could convert that bit into FILE_SHARE_DELETE. Then it would be easy for calls to either syscall.Open or os.OpenFile to cause the underlying call to use FILE_SHARE_DELETE if they really needed it.

This proposal assumes #32088 is declined. It should be considered withdrawn if #32088 is accepted.

@networkimprov

This comment has been minimized.

Copy link

@networkimprov networkimprov commented Oct 3, 2019

I would call this O_ALLOW_RENAME since that's the most common use of the flag. I'd also define it in package "os" so users don't need to write an open wrapper (with multiple implementations) and remember to always invoke that instead of os.OpenFile().

One may also wish to switch off FILE_SHARE_READ & _WRITE to prevent another caller from inadvertently blocking rename/delete by opening the file.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 4, 2019

Whatever the name, it's worth asking whether the new name should be defined on non-Windows systems, presumably with the value 0. The advantage of doing that would be that people who want to run on all systems could use the flag without relying on build tags. The disadvantage would be a flag that is odd and ineffective on non-Windows systems; on the other hand, that is already true of O_SYNC, or O_APPEND|O_WRONLY, on Windows systems.

@thaJeztah

This comment has been minimized.

Copy link
Contributor

@thaJeztah thaJeztah commented Oct 4, 2019

Agreed with @ianlancetaylor, that would make maintenance easier without having to breaking up code to platform-specific implementations

@mattn

This comment has been minimized.

Copy link
Member

@mattn mattn commented Oct 4, 2019

I agree with iant. We should consider to portability of the code but the meenless flag should not be added to UNIX. If we don't add the new flag on UNIX, the code for Windows must be compiled with build constraints. i.e. it is same thing that providing windows.Open() from golang.org/x/sys/windows.

@thaJeztah

This comment has been minimized.

Copy link
Contributor

@thaJeztah thaJeztah commented Oct 4, 2019

@mattn slightly confused, you say you agree with @ianlancetaylor, but then continue that you don't want to add the new flag on UNIX(/non-Windows)?

@networkimprov

This comment has been minimized.

Copy link

@networkimprov networkimprov commented Oct 5, 2019

@alexbrainman thoughts?

@mattn

This comment has been minimized.

Copy link
Member

@mattn mattn commented Oct 5, 2019

@thaJeztah I am simply considering for a reasonable and nondestructive way to make changes to the standard library. This proposal has a positive effect on Windows users, but it shows meaningless flag(s) to UNIX users.

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Oct 6, 2019

I think putting this functionality in a separate package works just fine (see #32088 (comment) for details). Why does it have to be in main Go repo?

If some insist, we could move that package somewhere under golang.ord/x/sys or x/exp.

Alex

@DmitriyMV

This comment has been minimized.

Copy link

@DmitriyMV DmitriyMV commented Oct 6, 2019

@alexbrainman
Because code duplicates the one which is in standard library. I don't think that another hack floating around (with forks and duplicates) is a really good idea. If that code is changed in the future - all forks would have to update, which is never going to happen.

The simple answer is - code maintenance. This is relatively cheap change, which doesn't require a lot of support in the future. I think we should be decreasing surface for bugs - we were bitten by it (not changing things) in the past (leap second and time package).

@mattn

This proposal has a positive effect on Windows users, but it shows meaningless flag(s) to UNIX users.

Don't we already have some of those (meaningless flags) for Windows users, which are meaningful on UNIX? I don't think people would mind adding new flag to the os package. But if they do, I'm open for adding this to syscall.

@andybons andybons changed the title syscall: define Windows O_ALLOW_DELETE for use in os.OpenFile proposal: syscall: define Windows O_ALLOW_DELETE for use in os.OpenFile Oct 7, 2019
@networkimprov

This comment has been minimized.

Copy link

@networkimprov networkimprov commented Oct 8, 2019

@alexbrainman could you restate here, since the other issue is so long, why it is that you don't support this proposal?

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Oct 8, 2019

Because code duplicates the one which is in standard library.

Sure it does. Whole github.com/alexbrainman/goissue34681 package is 375 lines long including docs and tests. It took me around 1 hour to write - I copied os.Open and os.OpenFile, and then copied whatever was missing. It took me longer to write test. It just Go code, there is very little magic there.

I don't think that another hack floating around (with forks and duplicates) is a really good idea.

I am not proposing forks or duplicates. I propose you use github.com/alexbrainman/goissue34681 package. I am happy to move the package, if people have better suggestions.

If that code is changed in the future - all forks would have to update, which is never going to happen.

If correspondent os package code change in the future, and that change affects github.com/alexbrainman/goissue34681 , then sure someone will have to report bug and fix the code. We should have some tests in github.com/alexbrainman/goissue34681 and run them regularly. Just like another package. But, going by my experience, I don't think there will be many changes like that.

The simple answer is - code maintenance.

What is proposed in here is not maintenance free either.

This change is at the heart of os package. How would new flag interact with all other code? How will new flag work with directories? With symlinks? What about file symlinks vs directory symlinks? What about directory junctions? What about network shares? And this flag needs to be explained to every single Go user. Including novices and non-Windows users. Should we recommend that they use the flag or not? When? How would their decision would affect their users? Are they in a position to make that decision?

So far I have seen this flag used to open files so they can be moved or deleted. Therefore I created github.com/alexbrainman/goissue34681 package with just Open and OpenFile functions. These are simple replacement of os.Open and os.OpenFile. All your remaining code should not change. And the package should also work for non-Windows use - it just call os.Open and os.OpenFile for them. I think github.com/alexbrainman/goissue34681 package is easier to use then new os pack age flag.

I think users who need that functionality should try github.com/alexbrainman/goissue34681 package before we even discuss adding new flag to os package.

could you restate here, since the other issue is so long, why it is that you don't support this proposal?

See above paragraph.

Alex

@DmitriyMV

This comment has been minimized.

Copy link

@DmitriyMV DmitriyMV commented Oct 8, 2019

then sure someone will have to report bug and fix the code.

Thats implying that someone is going to support this package forever. If one doesn't support it, then we face the situation with forks\duplicates.

This change is at the heart of os package. How would new flag interact with all other code? How will new flag work with directories? With symlinks? What about file symlinks vs directory symlinks? What about directory junctions? What about network shares? And this flag needs to be explained to every single Go user. Including novices and non-Windows users. Should we recommend that they use the flag or not? When? How would their decision would affect their users? Are they in a position to make that decision?

All of this is seems solvable by moving this flag to the syscall package.

I think github.com/alexbrainman/goissue34681 package is easier to use then new os pack age flag.
What about transitive dependencies which return *os.File?

If you are so against the idea of having as in standard, maybe we can move it into golang.org/x/sys/windows. That way we can at least ensure that people will maintain it in the future.

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Oct 9, 2019

Thats implying that someone is going to support this package forever. If one doesn't support it, then we face the situation with forks\duplicates.

Yes. Someone who uses this package has to support it, if it breaks. It could be you, if it is important to you. But like I said before, it is very little code, and it should not break often. If there are enough users of that package it shouldn't be a problem. Should it?

All of this is seems solvable by moving this flag to the syscall package.

I don't see how this flag can live in syscall package. Can you provide more details?

If you are so against the idea of having as in standard, maybe we can move it into golang.org/x/sys/windows. That way we can at least ensure that people will maintain it in the future.

I am fine moving this package into somewhere under golang.org/x. I am not sure about adding 2 new functions (Open and OpenFile) to golang.org/x/sys/windows. golang.org/x/sys/windows has a lot of code, they will get lost there. And golang.org/x/sys/windows package is only available on Windows. github.com/alexbrainman/goissue34681 is suppose to be straight replacement of os.Open and os.OpenFile - it should work on any OS.

Alex

@zx2c4

This comment has been minimized.

Copy link
Contributor

@zx2c4 zx2c4 commented Oct 9, 2019

This is a slippery slope. On Windows, CreateFile and NtCreateFile have a lot of very interesting options and overrides. Probably the existing set of parameters are sensible for the majority of normal files people open. For weird files in special conditions, users will probably want the full control of CreateFile or NtCreateFile, not just the stray O_ALLOW_DELETE exception because a user happened to report that particular need to our bug tracker. In other words, I agree here with @alexbrainman - if we want to provide this functionality, let's do it outside of os.OpenFile and instead stick it in x/sys/windows or an external package or somewhere sensible like that. I'd then deviate from Alex's idea of copying OpenFile precisely and instead I'd suggest a Windows-specific library where we can expose all the various insane switches and nobs that Windows offers.

tl;dr: 👎 on proposal.

@thaJeztah

This comment has been minimized.

Copy link
Contributor

@thaJeztah thaJeztah commented Oct 9, 2019

I think the problem was that currently, syscall.Open() handles flags it knows about (setting access, sa, and createmode based on those) then sets a hard-coded sharemode, and discards any other flag that was passed;

sharemode := uint32(FILE_SHARE_READ | FILE_SHARE_WRITE)

(thinking out loud) I wonder if it would be an option to have sharemode default to the current options, and append any flag that was not consumed by access, sa, or createmode. This would be similar to how the linux variant passes on whatever flags are passed;

func Open(path string, mode int, perm uint32) (fd int, err error) {
return openat(_AT_FDCWD, path, mode|O_LARGEFILE, perm)
}

(again, just thinking out loud)

@zx2c4

This comment has been minimized.

Copy link
Contributor

@zx2c4 zx2c4 commented Oct 9, 2019

I think the problem was that currently, syscall.Open() handles flags

syscall.Open returns a Handle, just like syscall.CreateFile.

If we're discussing syscall., what about just introducing x/sys/windows.OpenFile or syscall.OpenFile that wraps the relevant arguments of CreateFile, or perhaps better, NtCreateFile, and returns (*File, error). So something like:

func OpenFile(name string, access uint32, mode uint32, create uint32, attrs uint32, templateFile Handle, securityAttributes *SecurityAttributes) (*os.File, error)

Then people who want to do wild&crazy things with the Windows file opening routines can just use this and proceed as usual.

@jstarks

This comment has been minimized.

Copy link

@jstarks jstarks commented Oct 9, 2019

The motivation of this as well as the original proposal was to make it easier for Go software authors to write code that works cross-platform. We need to pass FILE_SHARE_DELETE to make Windows behave more like POSIX with respect to file deletion. There is general agreement that we cannot make this the default without regressing existing behavior, but the desire to have a clean, simple solution persists.

Any solution that requires extra dependencies, _windows.go files, build tags, etc. deviates from the goal of writing common code everywhere. I know from experience that it will be much easier to convince a random developer to add O_ALLOW_DELETE to their calls to os.OpenFile than to convince them to pull in an additional dependency with a custom fork of os.OpenFile, or to add Windows-specific .go files, just to get Windows to behave like everyone else.

I like the idea of putting a Windows-specific open routine in x/sys/windows to easily expose the wealth of CreateFile functionality! It would be great if we didn't have to replicate the complexity of calling syscall.CreateFile everywhere that needs Windows-specific functionality.

But I also think FILE_SHARE_DELETE is a special case worthy of inclusion in os.OpenFile, since it changes Windows behavior to be closer to all the other operating systems Go supports. I can't immediately think of any other CreateFile flags that do that.

@rsc

This comment has been minimized.

Copy link
Contributor Author

@rsc rsc commented Oct 9, 2019

Since #32088 has been declined, it seems like we should examine this carefully.

@alexbrainman seems to be saying it's not worth adding to package syscall when it can be done externally. But Alex, it's just a couple lines in syscall (that will be easy to maintain: O_ALLOW_DELETE literally means set FILE_SHARE_DELETE in the system call) versus having to maintain a 300+-line package elsewhere and also expect users to find it. Do you object strongly to adding just these few lines to package syscall?

@zx2c4, I understand your point about the many options, and maybe we should make it easier to get at CreateFile directly, but it does seem that this particular bit ("make Windows more like Unix file systems") is going to be wanted more often than most of the other settings.

Does anyone else object to adding these few lines?

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Oct 10, 2019

but the desire to have a clean, simple solution persists.

Any solution that requires extra dependencies, _windows.go files, build tags, etc. deviates from the goal of writing common code everywhere. I know from experience that it will be much easier to convince a random developer to add O_ALLOW_DELETE to their calls to os.OpenFile than to convince them to pull in an additional dependency with a custom fork of os.OpenFile, or to add Windows-specific .go files, just to get Windows to behave like everyone else.

I think github.com/alexbrainman/goissue34681 package fits your requirements perfectly. It is oddly named, but we can give it better name. And we can move it into more "supported" place.

But I also think FILE_SHARE_DELETE is a special case worthy of inclusion in os.OpenFile, since it changes Windows behavior to be closer to all the other operating systems Go supports.

But the flag is completely meaningless for majority of Go users.

os.OpenFile already has 8 flags. And some flags can be used in combinations. I think it is too many as is. I struggle to remember which flags do what when I review or write code.

So adding 9-th flag of O_ALLOW_DELETE makes things even more confusing for everyone. If I am Linux developer, should I use O_ALLOW_DELETE in my code and when?

Go is supposed to be simple to use.

But Alex, it's just a couple lines in syscall (that will be easy to maintain: O_ALLOW_DELETE literally means set FILE_SHARE_DELETE in the system call) versus having to maintain a 300+-line package elsewhere

The duplicate code is in main_windows.go, and it is 214 lines long.

And since when the line count is more important than clean API and maintainability?

We already have plenty of duplicate code - just compare os and path/filepath packages. We write tests, and make sure we don't break them.

Adding new os.O_ALLOW_DELETE flag won't be easier to maintain then new package. New flag needs to be documented, educated, made sure it works in unusual scenarios (see #34681 (comment) ). We would need to spend time fixing bugs and debugging flag usage. I am surprised I have to explain all these points to you.

and also expect users to find it.

I think this flag is only useful to narrow group of users. So, discover-ability is not a problem. I am certain, few people who needed this functionality, they already wrote their own code, just like I did myself.

Alex

@DmitriyMV

This comment has been minimized.

Copy link

@DmitriyMV DmitriyMV commented Oct 10, 2019

Adding new os.O_ALLOW_DELETE flag won't be easier to maintain than new package. New flag needs to be documented, educated, made sure it works in unusual scenarios (see #34681 (comment) ). We would need to spend time fixing bugs and debugging flag usage. I am surprised I have to explain all these points to you.

I'm sorry, but those points are not "sound" at all. All of this points can be used against implementing any sort of new functionality - additions to errors/context/net packages, generics, etc. I also don't see a problem with maintenance - we are not going to get more "windows" developers and contributors, if we are going to treat them as second class citizens, and force them to reimplement os functionality in side packages.

I also suspect there is a problem in communications, because ATM we are arguing about 3 different things:

  1. Having new flag is os/syscall package.
  2. Having new functionality in /x/sys side package.
  3. Not having it at all in Go repos.

I think it would be wise to start talking about those.

@networkimprov

This comment has been minimized.

Copy link

@networkimprov networkimprov commented Oct 10, 2019

I think @alexbrainman is right that a new flag is not a great solution.

I patch my syscall_windows.go as follows, and set syscall.Open_FileShareDelete = true once in apps that need it. This would not land upstream, but if enough folks apply it, that would demonstrate over time that there's no downside to making file_share_delete the default for os.Open/etc().

diff --git a/src/syscall/syscall_windows.go b/src/syscall/syscall_windows.go
index de05840..e1455d5 100644
--- a/src/syscall/syscall_windows.go
+++ b/src/syscall/syscall_windows.go
@@ -245,6 +245,8 @@ func makeInheritSa() *SecurityAttributes {
 	return &sa
 }
 
+var Open_FileShareDelete = false
+
 func Open(path string, mode int, perm uint32) (fd Handle, err error) {
 	if len(path) == 0 {
 		return InvalidHandle, ERROR_FILE_NOT_FOUND
@@ -270,6 +272,9 @@ func Open(path string, mode int, perm uint32) (fd Handle, err error) {
 		access |= FILE_APPEND_DATA
 	}
 	sharemode := uint32(FILE_SHARE_READ | FILE_SHARE_WRITE)
+	if Open_FileShareDelete {
+		sharemode |= FILE_SHARE_DELETE
+	}
 	var sa *SecurityAttributes
 	if mode&O_CLOEXEC == 0 {
 		sa = makeInheritSa()
@zx2c4

This comment has been minimized.

Copy link
Contributor

@zx2c4 zx2c4 commented Oct 10, 2019

FWIW, the .NET default appears to be only giving FILE_SHARE_READ, but not write. That generally fits my impression of how most Windows apps appear to work too.

@zx2c4

This comment has been minimized.

Copy link
Contributor

@zx2c4 zx2c4 commented Oct 10, 2019

Here's another curious insight from reading .NET code: they now support other operating systems, but of course all their constructors and whatnot take a FileShare share parameter because it was originally for Windows. So now they're trying to figure out what to do with that flag. Here's what they do:

        /// <summary>Initializes a stream for reading or writing a Unix file.</summary>
        /// <param name="mode">How the file should be opened.</param>
        /// <param name="share">What other access to the file should be allowed.  This is currently ignored.</param>
        private void Init(FileMode mode, FileShare share, string originalPath)
        {
            _fileHandle.IsAsync = _useAsyncIO;

            // Lock the file if requested via FileShare.  This is only advisory locking. FileShare.None implies an exclusive
            // lock on the file and all other modes use a shared lock.  While this is not as granular as Windows, not mandatory,
            // and not atomic with file opening, it's better than nothing.
            Interop.Sys.LockOperations lockOperation = (share == FileShare.None) ? Interop.Sys.LockOperations.LOCK_EX : Interop.Sys.LockOperations.LOCK_SH;
            if (Interop.Sys.FLock(_fileHandle, lockOperation | Interop.Sys.LockOperations.LOCK_NB) < 0)
            {
                // The only error we care about is EWOULDBLOCK, which indicates that the file is currently locked by someone
                // else and we would block trying to access it.  Other errors, such as ENOTSUP (locking isn't supported) or
                // EACCES (the file system doesn't allow us to lock), will only hamper FileStream's usage without providing value,
                // given again that this is only advisory / best-effort.

It looks like they try to make the Windows stuff sort of work by taking a shared lock when no locking is requested and an exclusive one when any locking is requested. Wild!

@cpuguy83

This comment has been minimized.

Copy link

@cpuguy83 cpuguy83 commented Oct 10, 2019

@alexbrainman You are asking every project that wants to use this option to copy several hundred lines of code that honestly I have no idea why the code is the way it is. All the special path handling and all... this even requires copying syscall.Open.

It's not a matter of "can" we do it. Yes we can. I did this exact thing to resolve our issue in moby/moby which you also copied in your repo.
"Should" we require all projects to make copies of this code?

The thing is, there's all the path handling is the os package, but then there's also actually modifying what's in syscall.

I'm all for copying the thing I need vs importing a package full of things I don't need but this is, in my opinion, too complicated to copy.

@mattn

This comment has been minimized.

Copy link
Member

@mattn mattn commented Oct 11, 2019

We should consider any cases carefully since people using syscall/os package are not only you. No easy to rollback code. No easy change the behavior. So we froze syscall package and we are working with golang.org/x repository. Adding Open_FileShareDelete is easy but it is hard to know what will break something before add.

Neither I nor you can confirm that all Go users want the FILE_SHARE_DELETE. If some package set Open_FileShareDelete = true, another package might be broken.

We want to know whether most of use-case can be done with x package.

@networkimprov

This comment has been minimized.

Copy link

@networkimprov networkimprov commented Oct 26, 2019

There are at least six other stdlib invocations of CreateFile() which should also set FILE_SHARE_DELETE or be able to. They're documented in #35150.

A flag specific to os.OpenFile() is an incomplete solution; we can't modify six other APIs to take a flag. And duplicating the code for all those APIs isn't sensible.

Suppose we provide an environment variable to enable/disable f.s.d. use in all CreateFile() calls? I'd argue for a variable that disables it.

EDIT: I was wrong about this; only CreateFile() calls which request certain kinds of file access need f.s.d.

@rsc

This comment has been minimized.

Copy link
Contributor Author

@rsc rsc commented Nov 6, 2019

We're already talking about the Windows-specific package syscall, so there's no need to say O_WINDOWS_something. It can be O_something.

We can easily add O_SHARE_DELETE now and add the others (O_SHARE_NONE, O_SHARE_READ, O_SHARE_WRITE) if anyone actually needs them. Are there real uses for them, or is this just hypothetical completeness?

Thanks @networkimprov for identifying the other CreateFile calls in #35150. I commented there asking whether we should set FILE_SHARE_DELETE unconditionally in those.

@rsc

This comment has been minimized.

Copy link
Contributor Author

@rsc rsc commented Nov 13, 2019

I don't have a clear sense of where people are on this.

Should we add syscall.O_ALLOW_DELETE on Windows,
or should we do nothing and close this?

@cpuguy83

This comment has been minimized.

Copy link

@cpuguy83 cpuguy83 commented Nov 13, 2019

IMO we need to be able to os.OpenFile(..., syscall.O_ALLOW_DELETE, ...).

I honestly find the duplicated flags in os confusing, particularly for non cross-platform concepts, but do not care if it is added to os or not.

@mpx

This comment has been minimized.

Copy link
Contributor

@mpx mpx commented Nov 14, 2019

Ideally the constant would exist across all platforms, otherwise build tags will be required for source with POSIX-like functionality that builds on Windows (I realise the functionality isn't strictly identical, but for some purposes it's enough).

@networkimprov

This comment has been minimized.

Copy link

@networkimprov networkimprov commented Nov 14, 2019

I agree with @alexbrainman; from #34681 (comment):

But the flag is completely meaningless for majority of Go users.

os.OpenFile already has 8 flags. And some flags can be used in combinations. I think it is too many as is. I struggle to remember which flags do what when I review or write code.

So adding 9-th flag of O_ALLOW_DELETE makes things even more confusing for everyone. If I am Linux developer, should I use O_ALLOW_DELETE in my code and when?

Go is supposed to be simple to use.

Patching the local syscall.Open() is a better solution than having to use OpenFile() with a windows-specific flag for any os.Open/Create() situation.

Eventually we'll prove that @rsc's suggestion in #32088 to set FILE_SHARE_DELETE by default in os.Open/Create/OpenFile() isn't harmful. After all, Microsoft requested that, and Rust does it.

@jstarks

This comment has been minimized.

Copy link

@jstarks jstarks commented Nov 14, 2019

It seems to me that there are three camps:

  1. People who want some way to use os.OpenFile but opt into FILE_SHARE_DELETE semantics. They do not want to fork os.OpenFile for this purpose. They want ordinary cross-platform Go code to be able to pass this flag without third-party packages. For them, adding O_ALLOW_DELETE (ideally with it defined to be 0 on non-Windows platforms) is a good proposal.
  2. People who want all opens on Windows to pass FILE_SHARE_DELETE. They don't want to have to change open code. They propose to change behavior for all Go programs and have asked for examples of Go programs that would be affected negatively by such a change. At least one such person wants this to be a runtime opt-in.
  3. People who have no interest in passing FILE_SHARE_DELETE in cross-platform Go code. They don't want any behavior change, and they do not want additional flags to os.OpenFile. They point out that passing this flag can already be achieved by calling syscall.CreateFile, and that passing this flag does not eliminate all cases where an open file cannot be deleted (especially on older Windows versions).

Those in camp 1 would like to see this proposal accepted. Those in camp 2 would like to see #32088 reopened and accepted. Those in camp 3 would like both proposals rejected.

I am against #32088. I have been convinced that such a breaking change could negatively affect existing Go programs, which violates the Go compatibility guarantees. It's unfortunate that Go 1.0 did not always pass FILE_SHARE_DELETE, but here we are. I have always agreed that making it a runtime opt-in would be a mistake.

However, I am in support of this proposal. I do not see why adding an additional O_ flag would be controversial. It seems to me that exposing this capability this will help reduce Windows-specific code in software written in Go, and will make it easier to fix some existing POSIX-oriented Go code to work on Windows. That seems valuable to me.

@slonopotamus

This comment has been minimized.

Copy link

@slonopotamus slonopotamus commented Nov 14, 2019

I believe that addition of FILE_SHARE_DELETE by default will break assumptions that otherwise are true. For example, while my process has an open file handle without FILE_SHARE_DELETE, my program is sure that file does exist under its name, can be os.Stat and etc. But this is no longer true with FILE_SHARE_DELETE.

Also, what happens to file handle created with FILE_SHARE_DELETE if other process does actually attempt to delete or rename file? Is it (file handle) still valid? Does it enter some new states visible through WinAPI (if yes, it can also break some programs)?

I'm obviously from camp 1 and againsts #32088.

@rsc

This comment has been minimized.

Copy link
Contributor Author

@rsc rsc commented Nov 20, 2019

It sounds like there is no consensus about this option either, so we can of course keep doing nothing.

I would be more persuaded by the "Microsoft requested that" argument for doing #32088 if the argument was coming from someone working on Windows, or maintaining the Go port to Windows, instead of someone whose bio says "Container+Linux guy at Microsoft". Sorry @jstarks but it makes perfect sense for the Linux guy to want that flag. (Most of us with Unix backgrounds think it makes sense, but the Windows users seem not to.)

Is there any more official statement from Microsoft that new programs on Windows should set this flag unconditionally in their calls?

@jstarks

This comment has been minimized.

Copy link

@jstarks jstarks commented Nov 20, 2019

@rsc I think there is some miscommunication--I agree with you and others that #32088 is the wrong choice for Go at this point. This is a change from my original position. I am advocating for this proposal, which is to add a flag to easily allow developers to easily opt into FILE_SHARE_DELETE in code that relies on this behavior and is intended to be cross platform.

Also regardless I think "Microsoft requested" is a weird way to put things, since I am not the Go representative for Microsoft in any mutually agreed capacity (if such a position can or should even exist). I am an engineering lead on the Windows Base team (kernel, file systems, virtualization). My team maintains and contributes to open source container-related components, hence my interest in Go.

I think you've convinced me to update my bio :).

@networkimprov

This comment has been minimized.

Copy link

@networkimprov networkimprov commented Nov 20, 2019

@jstarks I wonder whether you changed your mind because "a bad solution is better than none" :-)

But there is already a workable solution: patch the local stdlib so that an app can opt-in to f.s.d. There are other cases where GOOS=windows requires local patching:
File inode: https://groups.google.com/d/topic/golang-dev/raE01Fa2Kmo/discussion
os.O_SYNC flag: #35358

We wouldn't see these problems if Go was even half as widely deployed on Windows as Linux. Ideally someone at Google or Microsoft would be testing a selection of major open source Go projects on Windows, for each Go release.

I adopted Go because it supports Windows and MacOS. It's a productive toolkit, but I'm disappointed by Go's maturity on these platforms. For example, this MacOS data-loss bug went undetected until 2018: #26650

@rsc rsc added this to Active in Proposals Nov 27, 2019
@rsc

This comment has been minimized.

Copy link
Contributor Author

@rsc rsc commented Nov 27, 2019

But there is already a workable solution: patch the local stdlib

For what it's worth, even though you've been advocating for patching the standard library quite a lot recently, we generally don't consider that a workable solution to a problem.

For example, this MacOS data-loss bug went undetected until 2018: #26650

If you like that particular example read https://danluu.com/deconstruct-files/. Things are much worse pretty much everywhere.

@rsc

This comment has been minimized.

Copy link
Contributor Author

@rsc rsc commented Nov 27, 2019

@jstarks, thanks for the clarification about your role at Microsoft.

@alexbrainman and @mattn, we have someone who works on the kernel team at Microsoft saying that we should probably just set the flag unconditionally. Given this new information do you still think we should decline #32088? It seems like that would be less work all around, for us and for users, than adding O_ALLOW_DELETE.

@networkimprov

This comment has been minimized.

Copy link

@networkimprov networkimprov commented Nov 27, 2019

@rsc could we implement #32088 as an experimental opt-in? And if that's successful, enable it by default and provide an opt-out switch in later releases?

read https://danluu.com/deconstruct-files/. Things are much worse pretty much everywhere.

Everywhere? The article covers Unix, and has no references to Windows or MacOS/Darwin. NTFS does journaling, as does HFS+ since MacOS 10.3. So the PC platforms do pretty well -- aside from the fact that Darwin fsync() is a no-op, which foiled os.File.Sync().

Of course the Go project wouldn't want to recommend DIY patching to users, but it's a cost of doing business for some Go projects, and it's generally safer than copying stdlib code and inventing new APIs. Those are reasons to support it.

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Nov 28, 2019

@alexbrainman and @mattn, we have someone who works on the kernel team at Microsoft saying that we should probably just set the flag unconditionally. Given this new information do you still think we should decline #32088?

If you are referring to @jstarks , I think he said the opposite - from #34681 (comment)

I agree with you and others that #32088 is the wrong choice for Go at this point. This is a change from my original position. ...

Alex

@rsc

This comment has been minimized.

Copy link
Contributor Author

@rsc rsc commented Dec 4, 2019

I'm at a loss for a path forward here. I missed @jstarks's reversal above - thanks for flagging it @alexbrainman. There seem to be three options:

  1. Do nothing, like we have been. Removes will fail if the file is open.
  2. Set the flag always (#32088). Removes will succeed even if the file is open, although right now they apparently don't take effect until the file is closed. That could be (newly) confusing for Windows developers. On the other hand Windows has changed to the Unix behavior in the most recent versions.
  3. Add O_ALLOW_DELETE (this issue) and make users update all their code to add it, including rewriting os.Open calls to os.OpenFile in order to pass in the flag. But even if we do that, there is no way in the API to thread it into the various places that temporarily call CreateFile, such as os.Stat or os.Readlink. And if Go code has any handles, even temporarily, without this flag, then the removes are not going to succeed.

There is not consensus about 2, which is why we closed #32088.

But 3 seems like an incomplete solution at best. It will cause a lot of churn, forcing Windows developers to avoid helpers like os.Open, and still not solve the problem 100%. So it seems like not a good option.

It seems like if we can't come to a consensus on 2, then doing 1 until a better idea presents itself is the best path forward. Historically we've been willing to wait for the right idea (or a better understanding of the problem). It seems like that's where we are now.

@networkimprov

This comment has been minimized.

Copy link

@networkimprov networkimprov commented Dec 4, 2019

@rsc did you see this: "could we implement #32088 as an experimental opt-in? And if that's successful, enable it by default and provide an opt-out switch in later releases?"

That might be the better idea...

@cpuguy83

This comment has been minimized.

Copy link

@cpuguy83 cpuguy83 commented Dec 4, 2019

3 may be incomplete, but today we can't use os.Open or os.OpenFile already.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Dec 4, 2019

@networkimprov What would we learn from an experimental opt-in? The question is not what will happen with programs aware of the behavior change; it's what will happen with programs that are not aware. And programs that are not aware will not use the experimental opt-in.

@networkimprov

This comment has been minimized.

Copy link

@networkimprov networkimprov commented Dec 4, 2019

@ianlancetaylor the question is whether default f.s.d. conflicts with any third-party programs, not whether it could break Go programs. See #32088 (comment)

By letting projects opt-in to default f.s.d. (a variation of #32088) we provide a solution that doesn't automatically break anything, and can gather data about any conflicts with third-party programs. The experimental period could be 6, 12, or 18mos.

We can request that all projects intended for use on Windows test the opt-in when convenient.

Lacking that option, projects would likely take Alex's approach of re-inventing os.OpenFile() and only calling that on files that get renamed/removed while open. That can't yield much data re conflicts, and risks missing subsequent fixes in pkg os & syscall.

cc @alexbrainman this is the "experimental opt-in" I mentioned.

@mattn

This comment has been minimized.

Copy link
Member

@mattn mattn commented Dec 5, 2019

In my opinion, to request to use experimentals to anyone who does not really understand what should do is bit dangerous since some of developer who try to use the experimental will not revert the changes when the experimental is abandoned.

@networkimprov

This comment has been minimized.

Copy link

@networkimprov networkimprov commented Dec 5, 2019

If the experiment is abandoned, code to enable it would not compile. And the experimental mode could be enabled by a runtime env var (i.e. no code change) for anyone who's only testing it.

cc @alexbrainman

@networkimprov

This comment has been minimized.

Copy link

@networkimprov networkimprov commented Dec 6, 2019

To elaborate, you could either run a command
C:\...> set GOPOSIXRENAME=1 & app.exe

Or write code
func init() { os.Setenv(os.GOPOSIXRENAME, "1") }

to set file_share_delete in calls to os.Create/Open/OpenFile(). This functionality would be labeled experimental, meaning it might be withdrawn in a future release.

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Dec 8, 2019

@networkimprov you seems to ignore Ian's argument.

The question is not what will happen with programs aware of the behavior change; it's what will happen with programs that are not aware. And programs that are not aware will not use the experimental opt-in.

We all spent a lot of time investigating this change. But people that will be affected by this change have very little knowledge about this matter. They won't bother trying your experiment.

The experimental period could be 6, 12, or 18mos.

I don't believe anything will change in 18 month. FILE_SHARE_DELETE will still be here affecting existing programs. If Microsoft were really interested in changing this behavior, they could have started ignoring FILE_SHARE_DELETE in latest versions of Windows. I don't see that is happening. But then we don't need to change anything in Go.

Also having experiment in Go release is too high price to pay for this feature. We already have github.com/alexbrainman/goissue34681 - perfectly fine solution to your problem.

Alex

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