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

syscall: windows Open() should allow FILE_SHARE_DELETE #32088

Open
networkimprov opened this issue May 16, 2019 · 91 comments

Comments

Projects
None yet
@networkimprov
Copy link

commented May 16, 2019

On Linux & MacOS we can write this; on Windows it fails with a "sharing violation":

path := "delete-after-open"
fd, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, 0600)
if err != nil { panic(err) }
err = os.Remove(path)            // or os.Rename(path, path+"2")
if err != nil { panic(err) }
fd.Close()

If you develop on Windows and deploy to Linux etc, and your code relies on this undocumented GOOS=windows behavior of os.Rename() & .Remove(), it is broken and perhaps vulnerable. Note that package "os" has fifteen mentions of other Windows-specific behavior.

To fix this, syscall.Open() at https://golang.org/src/syscall/syscall_windows.go#L272
needs sharemode |= FILE_SHARE_DELETE

Erlang made it a default over six years ago: erlang/otp@0e02f48
Python couldn't adopt it due to limitations of MSVC runtime: https://bugs.python.org/issue15244

Given the Erlang experience, I believe that syscall.Open() should use file_share_delete by default, and syscall should provide a global flag to disable it where necessary (e.g. debugging).

Win API docs
https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-deletefilea
https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-createfilea

If there is a reason not to do this, it should be documented in os.Remove() & .Rename().

cc @alexbrainman
@gopherbot add OS-Windows

@bradfitz bradfitz added this to the Go1.14 milestone May 16, 2019

@bradfitz

This comment has been minimized.

Copy link
Member

commented May 16, 2019

@alexbrainman

This comment has been minimized.

Copy link
Member

commented May 18, 2019

syscall.Open() at https://golang.org/src/syscall/syscall_windows.go#L272
should use sharemode := ... | FILE_SHARE_DELETE

Why should it?

Alex

@networkimprov

This comment has been minimized.

Copy link
Author

commented May 18, 2019

FILE_SHARE_DELETE enables the code example above, which works on MacOS & Linux but fails on Windows. It's also necessary for:

fd, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, 0600)
defer fd.Close()
_, err = fd.Write(...)
err = fd.Sync()  // file now safe to share
err = os.Rename(path, path+"2")
_, err = fd.Read(...)
@alexbrainman

This comment has been minimized.

Copy link
Member

commented May 19, 2019

FILE_SHARE_DELETE enables the code example above, which works on MacOS & Linux but fails on Windows.

Why don't we adjust MacOS & Linux code instead?

It's also necessary for:

I don't understand what you are trying to say.

Alex

@mattn

This comment has been minimized.

Copy link
Member

commented May 19, 2019

@networkimprov You must call Remove after Close() on Windows.

path := "delete-after-open"
fd, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, 0600)
if err != nil { panic(err) }
fd.Close()
err = os.Remove(path)            // or os.Rename(path, path+"2")
if err != nil { panic(err) }
@networkimprov

This comment has been minimized.

Copy link
Author

commented May 19, 2019

@alexbrainman when doing reliable file I/O (as for databases), it's standard practice to create a file with a temporary name, write it, fsync it, and rename it.

Open files can be renamed or deleted by default on Unix. It seems like an oversight that the Windows flag for this capability is not set. I doubt we'll convince the Go team to change the way it works for Linux & MacOS :-)

@mattn pls apply the fix I described and try the code I posted.

@alexbrainman

This comment has been minimized.

Copy link
Member

commented May 19, 2019

I doubt we'll convince the Go team to change the way it works for Linux & MacOS :-)

I am fine the way things are now.

Alex

@networkimprov

This comment has been minimized.

Copy link
Author

commented May 19, 2019

Is there a rationale for omitting this common capability in Windows?

Can you provide a switch in syscall_windows.go so that we can select the Unix behavior at program start?

@mattn

This comment has been minimized.

Copy link
Member

commented May 19, 2019

I'm okay that we add new API or flags. But I have objection to change current behavior. Since FILE_SHARE_DELETE is not same as Unix behavior.

#include <windows.h>
#include <stdio.h>

int
main(int argc, char* argv[]) {
  HANDLE h = CreateFile("test.txt",
      GENERIC_READ | GENERIC_WRITE,
      FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
      NULL, OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
  getchar();
  char buf[256] = {0};
  DWORD nread;
  printf("%d\n", ReadFile(h, buf, 256, &nread, NULL));
  printf("%s,%d\n", buf, nread);
  return 0;
}

Comple this code on Windows, and try to run as test.exe. While this app is waiting hit-a-key, open new cmd.exe, delete "test.txt" like following.

image

The file can be deleted but remaining there while the process exists. So this change will not work well for your expected.

@networkimprov

This comment has been minimized.

Copy link
Author

commented May 19, 2019

I realize the directory entry isn't removed but the docs say

Subsequent calls to CreateFile to open the file fail with ERROR_ACCESS_DENIED.

So I don't understand your screen log.

Anyway, a switch like this would be fine:

syscall.OpenFileShareDelete = true
@ericlagergren

This comment has been minimized.

Copy link
Contributor

commented May 19, 2019

I’d like to mention that I’ve been bitten by this at work before.

Basically we had:

f, err := os.Open(...)
if err != nil { ... }
defer f.Close()

// lots of code

if err := os.Rename(...); err != nil { ... }

The code ran fine on our unix-based CI platforms, but exploded on Windows.

Assuming there aren’t any weird side effects, it would be nice if things just worked.

@bradfitz

This comment has been minimized.

Copy link
Member

commented May 19, 2019

IIRC, we've made a few adjustments to GOOS=windows & plan9 behavior in the past to more closely match Unix semantics. I wouldn't mind making this be another such case if the semantics are close enough. @mattn's comment, however, suggests the behavior is not close enough so it might not be worth it.

I don't want to see some global option, though. That just seems like a debugging nightmare.

@guybrand

This comment has been minimized.

Copy link

commented May 20, 2019

@bradfitz
It would probably not, please refer to my comment here
https://groups.google.com/forum/#!topic/golang-dev/R79TJAzsBfM
or if you prefer I can copy the content here, as there is also a possible solution, although I would not implement it as the default behavior, as this would not be consistent with how windows programs behave but rather a workaround to create a similar cross-os experience.

@networkimprov

This comment has been minimized.

Copy link
Author

commented May 20, 2019

@guybrand writes in the golang-dev thread:

my program is writing a file called "my.data", and then calls another program and does not wait for it to end ... this program lets say uploads this file which takes 10 seconds (lets call this other program "uploader") .
...
When my program calls .Remove on Windows (FILE_SHARE_DELETE is on):
...
uploader would receive an error telling it the file is removed (probably EOF).

Assuming "uploader" opens the file before "my program" calls os.Remove(), I think you contradicted the Win API docs:

DeleteFile marks a file for deletion on close. Therefore, the file deletion does not occur until the last handle to the file is closed. Subsequent calls to CreateFile to open the file fail with ERROR_ACCESS_DENIED.

Re "pairing an _open_osfhandle() of the CreateFile to an _fdopen", can you point to example code?

@mattn

This comment has been minimized.

Copy link
Member

commented May 20, 2019

If we add FILE_SHARE_DELETE, many programmer will mistakenly use it to simulate Unix behavior. In this case, you can make a function separated by build-constraints for each OSs. And it should return os.NewFile(), use FILE_SHARE_DELETE on Windows.

@networkimprov

This comment has been minimized.

Copy link
Author

commented May 20, 2019

make a function separated by build-constraints for each OSs... return os.NewFile(), use FILE_SHARE_DELETE on Windows

To retain the functionality of os.OpenFile() this plan appears to require reimplementing all of:

os.OpenFile()
openFileNolog()
openFile()
openDir()
newFile()
fixLongPath()
syscall.Open()
makeInheritSa()

@guybrand

This comment has been minimized.

Copy link

commented May 20, 2019

@networkimprov

@guybrand writes in the golang-dev thread:

my program is writing a file called "my.data", and then calls another program and does not wait for it to end ... this program lets say uploads this file which takes 10 seconds (lets call this other program "uploader") .
...
When my program calls .Remove on Windows (FILE_SHARE_DELETE is on):
...
uploader would receive an error telling it the file is removed (probably EOF).

Assuming "uploader" opens the file before "my program" calls os.Remove(), haven't you contradicted the Win API docs?

DeleteFile marks a file for deletion on close. Therefore, the file deletion does not occur until the last handle to the file is closed. Subsequent calls to CreateFile to open the file fail with ERROR_ACCESS_DENIED.

Re "pairing an _open_osfhandle() of the CreateFile to an _fdopen", can you point to example code?

I dont see a contradiction with WIndows API, please point what looks like a contridiction.

As for a code sample, I Googled a bit, and found this:
http://blog.httrack.com/blog/2013/10/05/creating-deletable-and-movable-files-on-windows/

BUT
Please note - this will only give you a nix-like behavior internally, any other external program working with it will break it (as it 99% using standard windows API

@guybrand

This comment has been minimized.

Copy link

commented May 20, 2019

@networkimprov
If you mean "this sounds like "dir my.data" would display the file, as far as I remember this was the behavior until windows .... XP or 7 (dont remember which) and changed since (perhaps the explorer just hides it somehow) - I can retest the next time I launch my windows env. (happens every few week...)

If you mean "this sounds like other processes with a handle to the file would still be able to read and write to the file", I would bet lunch the second you read write to such a file you do get an "EOF" style error - but again need to confirm to be 100% positive.

In either case, my point would be (at this point - I am taking "sides" in the "native like" vs " os-agnostic" point) - even if you implement _fdopen style solution, you would get inconsistency between your service and all other executables you collaborate with, so the use could only be in interaction with other go executables (or rare services that DO use fd's directly).

In other words - your app would be the "smartest kid in class" - that no other kid can understand.
Two examples from many I can think of:
Inputs:
My app downloads a file,the antivirus identifies its harfull and deletes/quarantines (==sort of rename) it, if I use fd - my app would still be able to do whatever it want with it (which is coll, but may end up hitting a virus...)
outputs:
my app pipes a file to another ("uploader" like) service, and deletes it, I even bothered and wrote a tester in go to see that all is working fine - and the test passes.
Now instead of my go test, I use filezilla, we transfter, dropbx API, whatever
it would fail/not behave in the same manner my test works...

Do you still think changing this to the default behavior makes sense ?

@alexbrainman

This comment has been minimized.

Copy link
Member

commented May 20, 2019

Is there a rationale for omitting this common capability in Windows?

I have never considered that question. I do not know.

The way Go files work on Windows is consistent with all other developers tools I have used in my life. It would be surprising to me, if Go files would work as you propose. I also suspect, it would break many existing programs.

Alex

@networkimprov

This comment has been minimized.

Copy link
Author

commented May 20, 2019

I also suspect, it would break many existing programs.

@alexbrainman I also suggested a switch to enable it, instead of changing the default.

@bradfitz there is syscall.SocketDisableIPv6, that's not really different from a flag to adjust syscall.Open() behavior.

@guybrand

This comment has been minimized.

Copy link

commented May 20, 2019

Since syscall_windows*.go states
func Open(path string, mode int, perm uint32) (fd Handle, err error) {
....
sharemode := uint32(FILE_SHARE_READ | FILE_SHARE_WRITE)

and type_windows*.go has
FILE_SHARE_DELETE = 0x00000004

All is quite ready, the only question is how to implement the flag.
I do not like the global option as well as its not explicit, and while a single developer may remember he has set os.DeleteUponLastHandleClosed = 1, its not a good practice for long term or multiple developer.
Other options would either be setting a specific reserved number for flags, like in:
fd, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE|os.DELETE_WHEN_FREED, 0600)
whereas DELETE_WHEN_FREED can even be 0 for env. other than windows,

Another option would be to use the perm parameter, which is not supported for windows, this may get a little awkward
fd, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, 777)
777 is reserved, so we would either need a 1777 or -777 ro support both systems
to make is readable DELETE_WHEN_FREED | 777

last option I can think of is os.OpenDeletableFile(
Which would os.OpenFile on nix's and
turn
sharemode := uint32(FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE)
on windows

all the above solutions are simple to implement, a little more time to test (cross os), just need a voter...

@havoc-io

This comment has been minimized.

Copy link

commented May 20, 2019

One way to support this behavior without changing the defaults or extending the API surface might be to just accept syscall.FILE_SHARE_DELETE in the flag parameter of os.OpenFile on Windows and fold it into the computed sharemode value. Since the syscall.O_* (and hence os.O_*) flags on Windows use made up values anyway, they could be engineered to avoid colliding with any Windows-specific flags that one wanted to include. Fortunately, they already avoid collisions with syscall.FILE_SHARE_DELETE.

In any event, I would strongly oppose changing the default behavior. Making FILE_SHARE_DELETE the default would put you in the same position as POSIX systems in terms of being unable to ensure race-free filesystem traversal. Having this flag be optional is why Windows doesn't need the equivalent of openat, renameat, readlinkat, etc.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

I haven't really thought about what we should do here, but I want to make one thing clear:

Can you provide a switch in syscall_windows.go so that we can select the Unix behavior at program start?

We will not be doing this. That would make it impossible for a single program to use different packages that expect different behavior.

@guybrand

This comment has been minimized.

Copy link

commented May 21, 2019

@havoc-io
Your suggestion would create error prone programs as syscall.FILE_SHARE_DELETE differs from POSIX std behavior.
Please note the authors of syscall_windows*.go were aware of the FILE_SHARE_DELETE flag (its in there) and decided using it would not be the preferred behavior.
Why?

lets take Liam's code, he
defer fd.Close()
deletes/renames
and then read/writes

so actual sort order is
open
mark as deleted
read/write (and probably cross process/cross go routine read write as well)
close

Current windows behavior alerts the developer "this is wrong", developer would probably push the delete to be defer'ed as well
If you Add FILE_SHARE_DELETE, windows would allow you to "mark as delete even though the file open" BUT another process/goroutine running concurrently that would try to access the file between the delete and the close (which is probably the longer task in this code) would fail
Result: instead of a consistent "you can do that" behavior - you would get a "sometimes - especially when there are many concurrent users it fails and I dont know why.
So your not solving the problem, just handling a specific use case on which the developer "only run that once"
My vote is for the consistent behavior of course...

How is that diff from Posix?
Once marked as deleted the file is no longer on the file table, it only exists as an fd, allowing another routine/process to create a file with the very same name.

I think we should stop looking for a "same solution" as there are differences we will not resolve within go (case sensitive filenames ? :) we will let Linux 5.2 do that...)

Altogether it looks like looking for a solution for a temporary file, if it is, windows supports GetTempFileName which can be considered as a standard solution for a file that is "used and then trashed" .

If on the other hand we wish to allow a developer to defer deletes in windows, that's possible, see my suggestions above, but the developer must take responsibility for that, and understand the consciousness - therefore must be a flag with a good name convention.

@networkimprov

This comment has been minimized.

Copy link
Author

commented May 21, 2019

The primary use for this feature is to rename an open file after creating it:

fd, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, 0600)
_, err = fd.Write(...)
err = fd.Sync()  // file now safe to share
err = os.Rename(path, shared_path)
// os.Close() at program exit

I don't know why you wouldn't add a flag os.O_WRNDEL (windows rename/delete; a no-op on other platforms), and document that mode's differences with Unix; that a deleted file remains in its directory, but can't be opened, until os.Close(). Also mention that moving the file to a temp dir before deleting it provides more Unix-like behavior.

@havoc-io

This comment has been minimized.

Copy link

commented May 21, 2019

@guybrand I think perhaps you're misunderstanding my proposal. I'm not advocating that FILE_SHARE_DELETE be enabled by default - in fact I'm opposing it for the reasons mentioned in the second paragraph of my comment. My proposal was for a mechanism that allows users to opt-in to FILE_SHARE_DELETE behavior (on a per-file basis) while still using the os package's file infrastructure. This proposal provides a mechanism for doing so without extending the API surface of any package.

I don't know why you wouldn't add a flag os.O_WRNDEL (windows rename/delete; a no-op on other platforms)

@networkimprov That's essentially what I'm proposing, except that there's no point in defining a new flag since a perfectly valid one already exists: syscall.FILE_SHARE_DELETE. The only implementation change would be to watch for this flag in syscall.Open on Windows and add checks to ensure that no future additions to syscall.O_*/os.O_* flags collide with this flag. The documentation for os.OpenFile could then be updated to reflect acceptance of this flag on Windows.

@guybrand

This comment has been minimized.

Copy link

commented May 21, 2019

@havoc-io sorry for the misunderstanding, this was my takeout from:
" ... to just accept syscall.FILE_SHARE_DELETE ... into the computed sharemode..."

Adding os.O_WRNDEL matches with my second suggestion apart from not being explicit enough for the developer in terms of "what would be the behavior", perhaps os.WADRNDEL - Windows allow deferred rename/delete) .

@mattn

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

I just want to know how may case exists to know whether we should implement new function(golang.org/x/sys?) for open/create a file with new attributes, Or change original behavior. Currently, as far as I can know from above comments:

  1. creating deletable(rename-able) file for logging.
  2. creating file for temporary file that will be deleted on close.

For 1, it can be provided from new function to create/open a file with the attributes. And we can set the file using logger.SetOutput(file).

For 2, also can create/open with new function.

BTW, I'm guessing #25965 is not related on this issue. Probably, it is limitation of Windows.

(In any case, we can not delete the directory where the file opened with this attribute exists)

@maruel

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

@mattn Can you explain the trade off between:

  • adding a completely new function in golang.org/x/sys
  • supporting a new flag (already defined!) to OpenFile(). [1]

I don't understand why we should favor the first instead of the second.

[1] I just realized I assumed here changing the behavior of os.OpenFile() but the issue calls out Open().

@mattn

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

First of all, syscall package is already locked down. So I'm looking for a way to fix this without changing the syscall package.

@olljanat

This comment has been minimized.

Copy link

commented Jun 4, 2019

BTW, I'm guessing #25965 is not related on this issue. Probably, it is limitation of Windows.

(In any case, we can not delete the directory where the file opened with this attribute exists)

@mattn that is not completely true. FILE_SHARE_DELETE flag will allow you move those files to another folder so you can remove original one. Here is simple PowerShell example of that:

# Create folder and open new file with FILE_SHARE_DELETE flag
New-Item -Type Directory -Path C:\folder1
$file = [System.IO.File]::Open("C:\folder1\test.txt", "Create", "ReadWrite", "Delete")

# Create temp folder and move all open files to there
New-Item -Type Directory -Path C:\folder1-removing
Get-ChildItem -Path C:\folder1 -Recurse | Move-Item -Destination C:\folder1-removing\

# Remove original folder
Remove-Item -Path C:\folder1

# Trigger removal for files on temp folder (NOTE! Those will exist on disk until they are closed).
Get-ChildItem -Path C:\folder1-removing -File -Recurse | Remove-Item -Force

# Close file and remove temp folder
$file.Close()
Remove-Item -Path C:\folder1-removing
@havoc-io

This comment has been minimized.

Copy link

commented Jun 4, 2019

First of all, syscall package is already locked down. So I'm looking for a way to fix this without changing the syscall package.

@mattn If it's only an issue of the API being frozen, then as I mentioned/demo'd above, syscall.Open could just be adjusted to understand syscall.FILE_SHARE_DELETE, solving the issue without changing the API.

If it's an implementation freeze, then the change could be made to golang.org/x/sys/windows's Open function and then vendored into the internal/syscall package. The file_windows.go code is already using functions from internal/syscall. The only inconsistency then would be that syscall.Open wouldn't understand this flag, but most people looking to access low-level Windows API functionality are using golang.org/x/sys/windows anyway, and probably using CreateFile instead of Open.

@networkimprov

This comment has been minimized.

Copy link
Author

commented Jun 4, 2019

The proposal that initiated the syscall package "lock-down" states:
The core repository will not depend on the go.sys packages

So changing x/sys wouldn't help callers of os.OpenFile(). But internal/syscall could add Open(), and os.OpenFile() would call it.

@maruel, the Docker project, and presumably many others, assumed Unix behavior for os.Rename() & .Remove() of opened files because package "os" doesn't mention Windows-specific behavior for them, yet has fifteen mentions of other Windows behavior.

@havoc-io

This comment has been minimized.

Copy link

commented Jun 4, 2019

The core repository will not depend on the go.sys packages
But internal/syscall could add Open(), and os.OpenFile() would call it.

@networkimprov Okay, I was under the mistaken impression that internal/syscall was a vendored subset of golang.org/x/sys, but either way I think that that's the right strategy (adding internal/syscall/windows.Open). Of course, this would be under the assumption that syscall.Open's behavior is immutable due to the freeze. Ideally it could be modified directly, potentially with corresponding changes in golang.org/x/sys/windows.Open.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

Although the syscall package is frozen, we can change it to fix bugs or address serious deficiencies. In particular we could change it to better support the os package.

But if people are calling syscall.Open directly, then they should be using the x/sys/windows package, not the syscall package.

@havoc-io

This comment has been minimized.

Copy link

commented Jun 5, 2019

But if people are calling syscall.Open directly, then they should be using the x/sys/windows package, not the syscall package.

I don't think anyone is looking to call syscall.Open directly - it's only the target of discussion because it underlies os.OpenFile (so that adding support for FILE_SHARE_DELETE in syscall.Open adds support for using the flag with os.OpenFile).

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

Thanks. It's fine to change the syscall package to support changes to the os package.

@maruel

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

Thanks @ianlancetaylor and @havoc-io . A more formal proposal from what I noted above:

  • Modify syscall.Open() with the explicit intent of allowing os.OpenFile() to accept FILE_SHARE_DELETE (and eventually FILE_FLAG_DELETE_ON_CLOSE as a potential follow up)
  • Update os.OpenFile() documentation to describe the new Windows-only flag.
  • Update os.OpenFile(), os.Rename() and os.Remove() documentation to clarify the difference in behavior between POSIX and Windows.

The third point is to address @networkimprov concern. I understand the challenge there, and while it shouldn't be the language's responsibility to describe how the OS works, I'm starting to agree with @mattn that these cases are subtle enough to warrant more documentation. I think the doc for Rename and Remove can be improved right away, no need to wait for any behavior change.

I'll willing to contribute on this if there's approval.

@alexbrainman

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

This leaves us with a per-call flag with a name that is clearly Windows-specific.

Do you propose new os.Open flag? Then the flag should be supported by all OSes - that is a whole point of os package - be OS independent. So what would semantics of that flag would be? And you would need some new tests that verify the flag works as documented.

I don't see why you cannot write whatever code you need using syscall package. I don't think your code will coexist well with other Windows programs. And maybe it is OK in special circumstances. But I don't want people using this functionality lightly - so it should not be part of standard library.

Alex

@guybrand

This comment has been minimized.

Copy link

commented Jun 6, 2019

@alexbrainman

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

@alexbrainman
please see this

I did look. I still don't see how all your proposed changes will work on other OSes? What tests will you have to implement your proposed change?

Alex

@guybrand

This comment has been minimized.

Copy link

commented Jun 6, 2019

quoting this

whereas DELETE_WHEN_FREED can even be 0 for env. other than windows

So - nothing to test on other OSes, the call for :
fd, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE|os.DELETE_WHEN_FREED, 0600)
in windows would be translating the const to 4
fd, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE|4, 0600)
While others (0)
fd, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE|0, 0600)

@alexbrainman

This comment has been minimized.

Copy link
Member

commented Jun 8, 2019

nothing to test on other OSes

I don't see how we could add new os package const or variable that is only usable on Windows.

c:\>go doc os
package os // import "os"

Package os provides a platform-independent interface to operating system
functionality. 
...
The os interface is intended to be uniform across all operating systems.
Features not generally available appear in the system-specific package
syscall.

Alex

@networkimprov

This comment has been minimized.

Copy link
Author

commented Jun 8, 2019

Alex the answer to your objection was first stated here #32088 (comment)

A test is trivial

_, err := os.OpenFile(path, os.O_CREATE | syscall.FILE_SHARE_DELETE, 0);
err = os.Rename(path, path+"2")

We're at the point where someone could submit a patch; I suspect @ianlancetaylor would accept it.

@bcmills

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

I investigated setting FILE_SHARE_DELETE as part of #32188, but found that setting it did not empirically reduce the rate of errors from os.Rename and io.ReadFile calls; a retry loop was still needed anyway.

I would be very interested to see a test that demonstrates a significant observable difference from setting this flag, because I don't really understand its implications myself.

@guybrand

This comment has been minimized.

Copy link

commented Jun 10, 2019

@networkimprov

This comment has been minimized.

Copy link
Author

commented Jun 10, 2019

@bcmills, reading #32188 I gather that you were seeing errors from os.Rename() on a file that was not open during the rename? I imagine one could outrun the NTFS journal, which I believe is written for all directory change ops, and thereby induce an error; but I've no idea what error it would be.

The request here to enable FILE_SHARE_DELETE in syscall.Open() is to allow os.Rename() to work on opened files. I have tested that flag with 1.12 on Win7, it works; and fails without it.

I have not specifically tested os.Rename() on windows "under load", but have not seen any unexpected errors from os.Rename() on opened files since enabling f.s.d. I'll report it if I do.

@bcmills

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

I gather that you were seeing errors from os.Rename() on a file that was not open during the rename?

Yes.

I've no idea what error it would be.

Empirically, the errors I'm seeing (from MoveFileEx, ReplaceFile, and/or are CreateFile) are ERROR_ACCESS_DENIED, ERROR_SHARING_VIOLATION, and ERROR_FILE_NOT_FOUND.

The request here to enable FILE_SHARE_DELETE in syscall.Open() is to allow os.Rename() to work on opened files. I have tested that flag with 1.12 on Win7, it works; and fails without it.

Right; the related problems I'm trying to solve are to allow concurrent calls to os.Rename to succeed, and to allow calls to io.ReadFile concurrent with os.Rename to succeed. Getting a single os.Rename to work with existing handles open may be a step in the right direction, but I don't think it's sufficient for the kind of use-cases we for which we employ the POSIX rename.

@olljanat

This comment has been minimized.

Copy link

commented Jun 10, 2019

I would be very interested to see a test that demonstrates a significant observable difference from setting this flag, because I don't really understand its implications myself.

@bcmills PowerShell example as I write that much faster than Go

New-Item -Type Directory -Path C:\folder1
for($i=0; $i -le 1000; $i++)
{
    $temp = [System.IO.File]::Open("C:\folder1\test.txt", "Create", "ReadWrite", "Delete")
    Set-Variable -Name "file$i" -Value $temp -Force
    $TempName = "C:\folder1\test.txt." + (New-Guid).Guid
    Rename-Item -Path C:\folder1\test.txt -NewName $TempName
    Remove-Item -Path $TempName -Force
}

After that finished there will be thousand test.txt.??? files under C:\folder1 but when you close PowerShell those files will be removed.

So what FILE_SHARE_DELETE flag actually does it allow you rename/move/remove open files but you still need make sure that destination file names are unique because they will exist on disk as long as handles are open.

@guybrand

This comment has been minimized.

Copy link

commented Jun 10, 2019

@networkimprov

This comment has been minimized.

Copy link
Author

commented Jun 10, 2019

allow calls to io.ReadFile concurrent with os.Rename to succeed

@bcmills I think that one should always work with the f.s.d. flag set, assuming it's not running concurrently with other threads attempting the same sequence on unrelated files. And it should usually fail without the flag.

That aspect of the test script correctly found a bug in Windows syscall.Open(). But no one else commenting in this thread wants it fixed :-( so yeah, patch the script.

@guybrand

This comment has been minimized.

Copy link

commented Jun 10, 2019

TLDR:
minor safe change to syscal_windows.go and works as expected.

explained:
So, here's what I've done, works as expected, and no risk (IMO)

  1. tweak syscal_windows.go
func Open(path string, mode int, perm uint32) (fd Handle, err error) {

sharemode := uint32(FILE_SHARE_READ | FILE_SHARE_WRITE | (mode & FILE_SHARE_DELETE))

the addition is : "| (mode & FILE_SHARE_DELETE)"
if the developer will not send mode & 4 set (why should he - this was never working before...) - nothing has changed, so a code like

os.OpenFile(filename,os.O_RDWR|os.O_CREATE, 0600)

will behave EXACTLY as it does before the change, only developers that will

os.OpenFile(filename,os.O_RDWR|os.O_CREATE  | 4 , 0600)

will have "feel" the change

  1. ran the code
package main

import (
	"fmt"
	"os"
)

func main() {
	filename := "./myfile"
	if f ,err := os.OpenFile(filename,os.O_RDWR|os.O_CREATE|4, 0600);err!=nil{
		fmt.Printf("Open file %s error : %s" , filename, err.Error())
	} else if _,err:= f.WriteString("bla bla") ;err!=nil{
		fmt.Printf("writing to %s returned error : %s" , filename, err.Error())
	} else if err := os.Remove(filename);err!=nil{
		fmt.Printf("removing %s returned error : %s" , filename, err.Error())
	}
}

and it worked

  1. ran the code without the |4 and it failed
  2. ran the code with a small addition:
package main

import (
	"fmt"
	"os"
)

func main() {
	filename := "./myfile"
	if f ,err := os.OpenFile(filename,os.O_RDWR|os.O_CREATE|4, 0600);err!=nil{
		fmt.Printf("Open file %s error : %s" , filename, err.Error())
	} else if _,err:= f.WriteString("bla bla") ;err!=nil{
		fmt.Printf("writing to %s returned error : %s" , filename, err.Error())
	} else if err := os.Remove(filename);err!=nil{
		fmt.Printf("removing %s returned error : %s" , filename, err.Error())
	} else if secondF ,err := os.OpenFile(filename,os.O_RDWR|os.O_CREATE|4, 0600);err!=nil{
		fmt.Printf("reOpen file %s error : %s" , filename, err.Error())
	} else {
		secondF.Close()
	}
}
 

and of course it failed on the secondF
But thats expected behavior in windows - no need to "load test" - there is no concurrency option on deferred .Remove, so I think we can safely add this half a line to syscal_windows, without compromising any existing code, and allowing only developers that really want to use this mode to be able to defer deletes.

all we need to do (in bold):
syscal_windows.go:
sharemode := uint32(FILE_SHARE_READ | FILE_SHARE_WRITE | (mode & FILE_SHARE_DELETE))

@olljanat

This comment has been minimized.

Copy link

commented Jun 10, 2019

I investigated setting FILE_SHARE_DELETE as part of #32188, but found that setting it did not empirically reduce the rate of errors from os.Rename and io.ReadFile calls; a retry loop was still needed anyway.

I would be very interested to see a test that demonstrates a significant observable difference from setting this flag, because I don't really understand its implications myself.

@bcmills now you can find unit/regression tests for this from olljanat@3828f1a currently it fails to error The process cannot access the file because it is being used by another process. on Windows and starts working after FILE_SHARE_DELETE flag is included to the code.

Note! that on Windows you still need that step which moves log example.log.1 file to random/unique name because other why renaming example.log to example.log.1 would fail to Access is denied. error even when FILE_SHARE_DELETE flag is enabled. That is platform specific thing which developer need to take care of.

@thaJeztah

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2019

/cc @jhowardmsft @jterry75 @jstarks @ddebroy FYI; you might be interested in this one as well.

@jterry75

This comment has been minimized.

Copy link

commented Jun 12, 2019

@kevpar - FYI since you are working around this as well

@networkimprov

This comment has been minimized.

Copy link
Author

commented Jun 12, 2019

If any folks joining the thread can voice reasons (e.g. existing code is broken) to make this the default, vs available via Windows-only flag in os.OpenFile(), do pipe up!

@olljanat

This comment has been minimized.

Copy link

commented Jun 14, 2019

@networkimprov IMO it should be handled with Windows-only flag because developer is needed to handle also some other Windows specific things anyway like on my log rotate example.

However as I can see that couple of Microsoft employees was invited to discussion so it is interesting to see if they thing this differently.

Have anyone btw started to develop new solution to this one? Any volunteers?

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