Use FILE_SHARE_DELETE for log files on Windows. #39974
Conversation
|
To be honest I don't understand how this would work even on theory? Sharemode is hardcoded on https://github.com/golang/go/blob/4219aec60ab473fa00f1092034ca801218a5dbe9/src/syscall/syscall_windows.go#L297 and that why there is that very long discussion about changing that hardcoded flag on golang/go#32088 Btw. You can use my unit test for this one https://github.com/olljanat/go/commit/3828f1a5d0ebb69b4c459d5243799ded36ac1ee8 if that passes on Windows you are good to go. |
b749478 to
b65a8b6
Compare
|
@olljanat You're right. I had to pull in a few other functions from |
This fixes issues where one goroutine tries to delete or rename a file while another goroutine has the file open (e.g. a log reader). Signed-off-by: Brian Goff <cpuguy83@gmail.com>
b65a8b6 to
a5f237c
Compare
| "github.com/pkg/errors" | ||
| ) | ||
|
|
||
| func openFile(name string, flag int, perm os.FileMode) (*os.File, error) { |
There was a problem hiding this comment.
Should we move this to pkg/system? In that package we also have some other windows-specific utilities (some of which) can be used as drop-in replacements for their default go implementations, e.g.
moby/pkg/system/filesys_windows.go
Lines 29 to 33 in e554ab5
There was a problem hiding this comment.
I'm intending this as a targeted workaround for a very specific and real issue rather than a generic solution.
It would be nice if go picked up some change to support this option.
There was a problem hiding this comment.
I'm intending this as a targeted workaround for a very specific and real issue rather than a generic solution.
Gotcha. I was thinking if other places would also benefit from this, having it in that package would make it a slightly bit more reusable; not a blocker though
It would be nice if go picked up some change to support this option
Yes, definitely
|
@cpuguy83 Now it it looks likely that it will use Only way to prevent that is rename file for some name which is not used by any other process before removing it. So if I understand this right it should be added to just before moby/daemon/logger/loggerutils/logfile.go Lines 218 to 222 in a36dfe7 |
|
@olljanat Do you have some example code to reproduce this? |
|
@cpuguy83 Hmm. It was mentioned on golang/go#32088 (comment) that defaults have been changed on latest versions of Windows and it really looks to be case at least on Windows 10, version 1903 so this simple PowerShell script works just fine on it but fails example on Windows Server 2019: $file = [System.IO.File]::Open("c:\temp\test.txt", "Create", "ReadWrite", "Delete")
Remove-Item -Path c:\temp\test.txt
$file2 = [System.IO.File]::Open("c:\temp\test.txt", "Create", "ReadWrite", "Delete") |
|
/cc @ameyag |
|
I'll see if this helps with master builds that fail on RS1 https://ci.docker.com/public/blue/organizations/jenkins/moby/activity/?branch=master |
|
We can cherry-pick once we see master is more stable with this 👍 |
…ows versions Signed-off-by: Olli Janatuinen <olli.janatuinen@gmail.com>
This (hopefully! 🤞 ) fixes issues where one goroutine tries to delete or rename a file
while another goroutine has the file open (e.g. a log reader).
Closes #39856
Closes #36801
Fixes #39274