golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
120.67k stars 17.33k forks source link

proposal: os: Create/Open/OpenFile() set FILE_SHARE_DELETE on windows #32088

Closed networkimprov closed 4 years ago

networkimprov commented 5 years ago

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 { ... }
err = os.Remove(path)            // or os.Rename(path, path+"2")
if err != nil { ... }
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

Microsoft recommends this be made the default: https://github.com/golang/go/issues/32088#issuecomment-502850674 Rust made it a default: https://doc.rust-lang.org/std/os/windows/fs/trait.OpenOptionsExt.html Mingw-w64 made it a default seven years ago: https://sourceforge.net/p/mingw-w64/code/HEAD/tree/stable/v3.x/mingw-w64-headers/include/ntdef.h#l858 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

~Therefore syscall.Open() should use file_share_delete by default, and syscall should provide both: a) a global switch to disable it (for any existing apps that rely on its absence), and b) a flag for use with os.OpenFile() to disable it on a specific file handle.~

Update after https://github.com/golang/go/issues/32088#issuecomment-532784947 by @rsc: a) os.Create/Open/OpenFile() should always enable file_share_delete on Windows, b) syscall.Open() on Windows should accept a flag which enables file_share_delete, and c) syscall on Windows should export a constant for the new flag.

The docs for os.Remove() should also note that to reuse a filename after deleting an open file on Windows, one must do: os.Rename(path, unique_path); os.Remove(unique_path).

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 commented 5 years ago

/cc @alexbrainman

alexbrainman commented 5 years ago

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

Why should it?

Alex

networkimprov commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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_ACCESSDENIED.

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

mattn commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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...

xenoscopic commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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.

xenoscopic commented 5 years ago

@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 commented 5 years ago

@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) .

alexbrainman commented 5 years ago

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

I am not interested. Thank you.

Alex

networkimprov commented 5 years ago

@guybrand rename of an open file is not deferred, I checked.

mattn commented 5 years ago

Sorry, I'm asking again what is your suggestion? delete opening file? rename opening file? both?

networkimprov commented 5 years ago

Three of us are now suggesting a new flag, e.g.

fd, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE| os._WRNDEL, 0600)

fd, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE| syscall.FILE_SHARE_DELETE, 0600)
xenoscopic commented 5 years ago

Three of us are now suggesting a new flag

I think that a new flag is inappropriate, especially one that isn't easily parsable by someone reading the code. Using syscall.FILE_SHARE_DELETE would be far more explicit and clear, immediately signaling to the reader that something platform-specific is happening.

This is already allowed on POSIX platforms. The are many POSIX-specific (and even platform-specific) open flags that aren't incorporated into the os package. For example, it wouldn't make sense to add something like os._DEVTONLY to support Darwin's O_EVTONLY flag, because you can just pass the flag from the syscall package directly to os.OpenFile, e.g.:

os.OpenFile(..., os.O_RDONLY | syscall.O_EVTONLY, ...)

In this case, the platform-specific flag will be passed down to the underlying open system call.

If FILE_SHARE_DELETE behavior is truly something that people need, then I think the answer is simply to make os.OpenFile able to similarly thread flags down to the underlying CreateFileW call on Windows. At least for FILE_SHARE_DELETE.

An example implementation can be found here.

mattn commented 5 years ago

@networkimprov

Three of us are now suggesting a new flag, e.g.

What do you expect the flag?

guybrand commented 5 years ago

@guybrand rename of an open file is not deferred, I checked.

Right, but both need FILE_SHARE_DELETE in order to be allowed while file is open If you are referring to my suggested terminology, we can os.WARNDFDEL - Windows allow rename and deferred delete is a little too long, I myself would use an acronym at all WINDOWS_ALLOW_OPENNED_FILE_RENAME_OR_DEFFERED_DELETE is better IMO.

xenoscopic commented 5 years ago

@guybrand I really don't see the value of adding a new platform-specific flag with a complex name and opaque behavior to the os package when a perfectly good flag for this has been around for decades (FILE_SHARE_DELETE). Using syscall.FILE_SHARE_DELETE as the flag for this is far more clear and explicit. As I mentioned above, there are many POSIX-specific and platform-specific flags that aren't added to the os package but are still accepted by it.

networkimprov commented 5 years ago

syscall.FILE_SHARE_DELETE is windows-only; we need something that's a no-op elsewhere. Maybe it should appear in pkg os, with a WIN or WINDOWS prefix?

@mattn pls see the patch above from @havoc-io

xenoscopic commented 5 years ago

we need something that's a no-op elsewhere.

@networkimprov The flag doesn't need to exist elsewhere. Your Windows code will just look like:

import (
    "syscall"
    "os"
)

file, err := os.OpenFile(..., os.O_RDWR | syscall.FILE_SHARE_DELETE, ...)

Yes, this code will need to be gated on the platform, but you'll probably need to be doing that anyway since the behavior with this flag won't match that of POSIX systems. Or, if you want the convenience of having the same code on all platforms, you can define your own constant (with a value of syscall.FILE_SHARE_DELETE on Windows and 0 on non-Windows systems) and pass it to os.OpenFile. This is exactly what you'd do if you wanted to bring in a platform-specific O_* flag that wasn't part of the os package (e.g. O_EVTONLY on Darwin or O_ASYNC on Linux).

guybrand commented 5 years ago

When I write code I expect it to be cross platform, FILE_SHAREDELETE does not exist in syscall* just the windows one (anf therefore will not compile on other systems).

Can we use 0x4 const, as its : FILE_SHARE_DELETE = 0x00000004

Apart from being ugly, netbsd_arm O_NDELAY = 0x4 O_NONBLOCK = 0x4

So using the 0x4 would create the code that differently in netbsd_arm.

So either we add FILE_SHARE_DELETE to all platforms, windows ones will be 0x4 others will be 0x0 and thus "discard" it, or create a specific flag for this issue.

AND anyhow we must change syscall_windows* func open( ... sharemode := uint32(FILE_SHARE_READ | FILE_SHARE_WRITE | isDeleteOn ) where isDeleteOn will check if mode includes the new flag

xenoscopic commented 5 years ago

@guybrand

When I write code I expect it to be cross platform

I would caution you that just because the same os.OpenFile call compiles on multiple platforms, it doesn't mean that your code is cross-platform. There are a number of security considerations on Windows that you would need to take into account when using FILE_SHARE_DELETE, and the semantics of file lifecycles are so different between POSIX and Windows systems that it's really hard to imagine a scenario where your file handling code wouldn't look at least a little bit different between the platforms. Using FILE_SHARE_DELETE essentially puts you in a pre-POSIX.1-2008 situation, without any way to ensure race-free filesystem traversal (and without the benefit of POSIX's post-unlink file access for open files).

Anyway, if you want a flag that has a value of syscall.FILE_SHARE_DELETE on Windows and 0 on other platforms, you can still easily do that with your own constant definition that's controlled by build tags, and then your main code that calls os.OpenFile can be the same on all platforms (using that custom flag).

There are many platform-specific flags that exist in other platforms' syscall packages, but they can't all be exposed by the os package and no-op'd on platforms where they aren't supported. If you want platform-specific code and behavior, it's necessary to reach for the syscall package.

The only thing that the Go standard library should (potentially) do here is support syscall.FILE_SHARE_DELETE in syscall.Open (and consequently os.OpenFile) on Windows.

mattn commented 5 years ago

As I mentioned in above, If we add FILE_SHARE_DELETE, many programmer will mistakenly use it to simulate Unix behavior. But it is not good. For example, please think about case of creating application to watch file exists. It is supporsed to be file must not be found from another applications if an application delete a file. But FILE_SHARE_DELETE do only mark to delete in later. The file is remaining. On UNIX, it works well, but Windows doesn't. Please do NOT use FILE_SHARE_DELETE for simulating UNIX behavior.

xenoscopic commented 5 years ago

@mattn I agree with your concerns - people may try to use the flag as a convenience to make Windows code "work the same" as POSIX systems without understanding the subtle implications. However, I think that if they have to go to the syscall package to access that flag, they might have taken the time to understand what it does.

networkimprov commented 5 years ago

@mattn, for Unix-like behavior, apps would need 2 extra lines of platform-neutral code:

fd, err := os.OpenFile(path, ... | syscall.FILE_SHARE_DELETE, 0600)
...
tmp_path := mkTempName()
err = os.Rename(path, tmp_path)  // works immediately; path is now available
err = os.Remove(tmp_path)

Documenting this requires one sentence in the docs for os.OpenFile().

Pls don't make me distribute a patch to pkg syscall that everyone who works on my windows code must apply! See also https://github.com/golang/go/issues/32088#issuecomment-493876119.

guybrand commented 5 years ago

I think we all agree on most of the facts, just phrase it differently so I'll try to summarize, and set up course of action:

  1. Agreed : Windows and POSIX differ in the above behavior and we should not try to aim for same behavior.
  2. Agreed : the request to allow FILE_SHARE_DELETE as a flag is useful
  3. Suggestion : let change the ticket to "support FILE_SHARE_DELETE" so it would sound like a feature request rather than a bug
  4. Mostly agreed : The feature should be a developer flag at a program level, not go base package.

Action Items:

  1. change syscall_windows.go func open( ... sharemode := uint32(FILE_SHARE_READ | FILE_SHARE_WRITE | (mode & FILE_SHARE_DELETE) )
  2. Developer that would like to utilize this behavior would either file, err := os.OpenFile(..., os.O_RDWR | syscall.FILE_SHARE_DELETE, ...) or if they would like their program to be cross platform would create an internal flag of:

mycode_windows.go const OPENFILEFLAG = syscall.FILE_SHARE_DELETE mycode_others.go const OPENFILEFLAG = 0

and then use: file, err := os.OpenFile(..., os.O_RDWR | OPENFILEFLAG, ...)

If this is agreed, the fix is minimal (one liner), with very low risk

Let vote on this, and we can quickly fix this

mattn commented 5 years ago

Agreed : the request to allow FILE_SHARE_DELETE as a flag is useful

I don't agree with this yet since I don't understand what is the purpose of this issue?

networkimprov commented 5 years ago

@mattn pls see https://github.com/golang/go/issues/32088#issuecomment-494305074

guybrand commented 5 years ago

Purpose : allow developers to rename or delete opened files.

What's missing : syscall_windows.go currently hard coded : func Open(path string, mode int, perm uint32) (fd Handle, err error) { ... sharemode := uint32(**FILE_SHARE_READ | FILE_SHARE_WRITE**)

Proposed change: When developer passes a mode parameter with bitwise 4 (FILE_SHARE_DELETE) turned on, the sharemode will change accordingly by sharemode := uint32(FILE_SHARE_READ | FILE_SHARE_WRITE | **(mode & FILE_SHARE_DELETE)** )

mattn commented 5 years ago

I'm still wondering why you don't call Close() before Rename() or Remove(). You can read code of standard library of Go. The files are closed by Close() before Rename() or Remove(). Why you must call Close() after Renamoe/Rename ?

guybrand commented 5 years ago

I'm not sure what is the business case @networkimprov is considering, my takeout on this would be a temp shared file - my app and another app are using this file, I want to make sure (even if my app crashes) this file will not exist anymore after I'm done - so I create it, Remove() it and when I either close it or my app is closed - the file is remove.

but it looks like @networkimprov wants to use the Rename(), so probably a diff use case.

networkimprov commented 5 years ago

As mentioned in https://github.com/golang/go/issues/32088#issuecomment-494305074 we may not need to close a renamed file until program exit.

@mattn, pls don't insist that we should never use an OS feature that C and C++ developers can use.

xenoscopic commented 5 years ago

I want to make sure (even if my app crashes) this file will not exist anymore after I'm done

@guybrand A defer statement isn't guaranteed to run in every type of crash, so if you're trying to ensure file deletion, then deferring an os.Remove operation isn't a reliable way to go about it. If you want a file that's automatically deleted, a temporary file that the system deletes is a better option. If you want a file location that's shared by two programs, it should be coordinated by locks (e.g. fcntl locks on POSIX and LockFileEx/UnlockFileEx on Windows), not file existence.

As mentioned in #32088 (comment) we may not need to close a renamed file until program exit. @mattn, pls don't insist that we should never use an OS feature that C and C++ developers can use.

@networkimprov A rename-after-open operation is a valid use case, but if you're the program holding the file HANDLE, can't you already do that? The only purpose of FILE_SHARE_DELETE would be to allow other processes to rename the file while you're holding it open. In any case, you can use FILE_SHARE_DELETE now, you just need to manually invoke CreateFileW and pass the result off to os.NewFile. You can find an example of what this looks like here. The resulting HANDLE can just be passed to os.NewFile.

@mattn I'm starting to agree with you that this flag will be more of a footgun than a useful tool. Just to be clear, I don't actually want this flag, and the only arguments I can see for its existence are completeness and parity of control between POSIX and Windows. On the other hand, as I mentioned above, it's also relatively easy to manually invoke CreateFileW (with this flag specified) and then hand the result off to os.NewFile, so maybe there is no need to add support for it.