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

Closed
rsc opened this issue Oct 3, 2019 · 62 comments
Closed

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

rsc opened this issue Oct 3, 2019 · 62 comments

Comments

@rsc
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

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
Copy link
Contributor

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

@mattn
Copy link
Member

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
Copy link
Contributor

@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
Copy link

@alexbrainman thoughts?

@mattn
Copy link
Member

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
Copy link
Member

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
Copy link

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
Copy link

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

@alexbrainman
Copy link
Member

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
Copy link

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link

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
Copy link

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link

@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
Copy link
Member

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
Copy link

@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
Copy link
Member

@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
Copy link
Contributor Author

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 (proposal: os: Create/Open/OpenFile() set FILE_SHARE_DELETE on windows #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
Copy link

@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
Copy link

cpuguy83 commented Dec 4, 2019

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

@ianlancetaylor
Copy link
Contributor

@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
Copy link

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
Copy link
Member

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
Copy link

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
Copy link

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
Copy link
Member

@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

@rsc
Copy link
Contributor Author

rsc commented Dec 11, 2019

I'm still at a loss for a path forward here, as I said last week in #34681 (comment).

Here's another idea. We could decide to try #32088 by landing the change on day 1 of the Go 1.15 cycle and then wait the 3 months to see if any problems are reported before deciding to keep it. If we decided to keep it at the start of the freeze and then any serious problems arose during the freeze (the following 3 months) we could still back it out. So that would be 6 months in which people could try it, including a beta and release candidate.

If we did that trial period, would that ease some of your concerns, @alexbrainman and @mattn?

@alexbrainman
Copy link
Member

Here's another idea. We could decide to try #32088 by landing the change on day 1 of the Go 1.15 cycle and then wait the 3 months to see if any problems are reported before deciding to keep it. ...

If we did that trial period, would that ease some of your concerns, @alexbrainman and @mattn?

Not really. Nothing you said in #32088 (comment) changed.

Alex

@networkimprov
Copy link

I've tried to summarize all the options offered to date. Please point out anything I've missed.

  1. Default FILE_SHARE_DELETE
    pro
    - Works for os.Create/Open/OpenFile (no user code changes)
    con
    - May silently impact 3rd party programs (not yet identified)

  2. Default via experimental switch
    pro
    - Works for os.Create/Open/OpenFile (no user code changes)
    con
    - Apps that enable the switch may silently impact 3rd party programs

  3. User-patched syscall.Open
    pro
    - Works for os.Create/Open/OpenFile (no user code changes)
    con
    - Must re-patch after upgrade
    - Apps that enable the patch may silently impact 3rd party programs

  4. Non-stdlib API which duplicates os.Create/Open/OpenFile
    pro
    - Supported in x/sys/windows (?)
    con
    - Unfamiliar API for ordinary file ops (user code changes)
    - May miss fixes to stdlib
    - App-wide use of new API may silently impact 3rd party programs

  5. New os.OpenFile flag O_ALLOW_DELETE
    pro
    - Supported in stdlib
    con
    - Doesn't work for os.Create/Open (user code changes)
    - Meaningless on Unix
    - Files opened this way may silently impact 3rd party programs

@guybrand
Copy link

Comments:
As for

New os.OpenFile flag O_ALLOW_DELETE

  • Doesn't work for os.Create/Open (user code changes)

Not sure what you mean, if we add the O_ALLOW_DELETE (@rsc : "(maybe 0x100000)") to the flags param, we are safe .

Also, the above summary should consider how to handle the switched above on "old" (but most) windows versions that do no support per option, e.g.:

Default FILE_SHARE_DELETE - probably no indication
Default via experimental switch - probably no indication, unless its a "smart switch" that will output a runtime warning...no one reads those, but debugging wise it can help
User-patched syscall.Open - should probably return an error, otherwise behavior would not be as expected.
Non-stdlib API which duplicates os.Create/Open/OpenFile- should probably return an error, otherwise behavior would not be as expected.
New os.OpenFile flag O_ALLOW_DELETE- should probably return an error, otherwise behavior would not be as expected.

@networkimprov
Copy link

Re (5) os.Create/Open don't take a flag argument, so would have to be replaced with os.OpenFile.

All Windows versions supported by Go recognize file_share_delete.

@rsc
Copy link
Contributor Author

rsc commented Jan 8, 2020

All Windows versions supported by Go recognize file_share_delete.

But they don't do the same thing with it. Older ones simply say they deleted the file but don't actually delete the name (it still shows up in directory listings) until the open handles are closed. Newer ones delete the name eagerly, like Unix, not lazily like the old Windows versions.

Overall it seems like there is still no consensus on what to do here, just like in #32088 (comment).

Given the lack of consensus, this seems like a likely decline.

Leaving open for a week for final comments.

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Jan 8, 2020
@networkimprov
Copy link

There's a fundamental stdlib gap here; some issue should be open to track it. I can file a new issue listing the above 5 options which have been considered to date... Thoughts?

@rsc
Copy link
Contributor Author

rsc commented Jan 15, 2020

We have behavior that some people think is a bug. Based on multiple discussions, we've decided not to make a change in the behavior. To me, that sounds like a closed issue. This one and the other will still be linkable; they're just closed.

No change in consensus (that is, still no consensus to do anything), so declining.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests