Closed bradfitz closed 5 years ago
bcmills
commented
5 years ago Do we have a reason to suspect that this is a regression?
Given that it manifests as a failure (not data corruption) when running go commands concurrently (already a bit of a power-user condition), this doesn't seem like it needs to be a release-blocker.
bcmills
commented
5 years ago @alexbrainman, do you have any tips on figuring out the actual error code from Windows error messages?
grep and Google search are failing me on this one. (Lots of advice for application consumers, but the developer documentation eludes me...)
bradfitz
commented
5 years ago Frequent tests flakes are always a release blocker.
We can disable the test if it's not fixed soon.
bcmills
commented
5 years ago Frequent tests flakes are always a release blocker.
Agreed, but you didn't mention how frequent it is. 🙂
I see ~5 flakes on the first page of the build dashboard, so I agree that this is reasonable as a release-blocker.
bradfitz
commented
5 years ago @bcmills, trybots too. I haven't gone over those logs, but I've seen enough myself now to be annoyed & filed this bug.
alexbrainman
commented
5 years ago @alexbrainman, do you have any tips on figuring out the actual error code from Windows error messages?
It is ERROR_SHARING_VIOLATION (search https://docs.microsoft.com/en-us/windows/desktop/debug/system-error-codes--0-499- for The process cannot access the file because it is being used by another process.).
grepand Google search are failing me on this one. (Lots of advice for application consumers, but the developer documentation eludes me...)
Same. But I went to the page where all windows errors are, and found your error text there.
If I google for windows errors, I find what I want pretty quickly:
https://docs.microsoft.com/en-us/windows/desktop/debug/system-error-codes--0-499-
https://docs.microsoft.com/en-us/windows/desktop/debug/system-error-codes
I did not look at the code, but the fact that we get ERROR_SHARING_VIOLATION while opening go.mod, suggests that something unusual happens. Normally I would expect this error when trying to open/delete/move file for executable that is running. The fact it happens with go.mod file - maybe some locking code of yours is not working properly. I am, probably, wrong.
Alex
gopherbot
commented
5 years ago Change https://golang.org/cl/179859 mentions this issue: cmd/go/internal/renameio: add a concurrent stress test
bcmills
commented
5 years ago The fact it happens with go.mod file - maybe some locking code of yours is not working properly.
That was my first guess too, but the go.mod file in particular is manipulated using renameio rather than lockedfile. I think we need to stress-test renameio to nail it down — could be a bug in renameio or in ioutil.ReadFile, or just a bizarre Windows quirk that we have to work around at individual call sites.
bcmills
commented
5 years ago My stress-test successfully reproduced the failure mode, along with a few other related failure modes, on the first attempt. The renameio hypothesis seems to be confirmed.
https://storage.googleapis.com/go-build-log/51ca1bc6/windows-amd64-2016_af1db195.log
bcmills
commented
5 years ago To summarize, it seems that os.Rename is not actually atomic on Windows under load.
(See previously #8914; see also #22397.)
bcmills
commented
5 years ago os.Rename on Windows is implemented via internal/syscall/windows.Rename, which uses the Windows MoveFileEx system call with the MOVEFILE_REPLACE_EXISTING flag. That appears not to be atomic.
Per this article, it appears that we could use ReplaceFile as an alternative.
It is not obvious to me whether os.Rename itself should be changed to use ReplaceFile, but we're probably too deep into the 1.13 freeze to do that right now anyway.
alexbrainman
commented
5 years ago To summarize, it seems that
os.Renameis not actually atomic on Windows under load.
What do you mean atomic ?
If you refer to
renameio_test.go:39: rename C:\Users\gopher\AppData\Local\Temp\1\renameio091369927\blob.bin336963765.tmp C:\Users\gopher\AppData\Local\Temp\1\renameio091369927\blob.bin: Access is denied.in your log, then the Access is denied error could be for different reason.
Also
renameio_test.go:45: open C:\Users\gopher\AppData\Local\Temp\1\renameio091369927\blob.bin: The process cannot access the file because it is being used by another process.could be because ioutil.ReadFile does not work properly while your renameio.WriteFile is in progress.
I am not familiar enough with your problem, but do you really need file locks? Do you know that you can use memory sync objects, like
https://docs.microsoft.com/en-us/windows/desktop/api/synchapi/nf-synchapi-createmutexexw
or
https://docs.microsoft.com/en-us/windows/desktop/api/synchapi/nf-synchapi-createmutexexw
and many others? These will provide you with whatever sync functionality you require across single computer. I also suspect they will be faster than working with files, because they are pure sync objects.
Alex
bcmills
commented
5 years ago What do you mean atomic ?
An atomic file-rename has the property that any read of the file sees either its contents before the rename or after the rename, never some third state (such as partially-zeroed, truncated, or inaccessible).
could be because ioutil.ReadFile does not work properly while your renameio.WriteFile is in progress.
Yes, that's exactly what I mean about it not being atomic: if Rename were atomic, then ioutil.ReadFile should work concurrently with renameio.WriteFile.
It isn't obvious to me whether ReplaceFile is atomic in that sense either. If it is not, then perhaps we will need to use the side-lock to guard go.mod and go.sum edits on Windows. (We do not currently do any locking for those files, instead relying on the atomicity of renameio operations.)
bcmills
commented
5 years ago the
Access is deniederror could be for different reason.
Yes, that particular error is probably another manifestation of #30789. The cannot access the file because it is being used by another process error is the more interesting one in terms of atomicity and go.mod updates.
gopherbot
commented
5 years ago Change https://golang.org/cl/180218 mentions this issue: internal/syscall/windows: add ReplaceFile and a Replace wrapper around it
gopherbot
commented
5 years ago Change https://golang.org/cl/180219 mentions this issue: cmd/go/internal/renameio: use ReplaceFile instead of MoveFileEx on Windows
gopherbot
commented
5 years ago Change https://golang.org/cl/180317 mentions this issue: syscall: on Windows, set FILE_SHARE_DELETE in Open
alexbrainman
commented
5 years ago An atomic file-rename has the property that any read of the file sees either its contents before the rename or after the rename, never some third state (such as partially-zeroed, truncated, or inaccessible).
Then I don't see how do you know that Windows MoveFileEx system call with the MOVEFILE_REPLACE_EXISTING flag is not atomic.
Yes, that's exactly what I mean about it not being atomic: if
Renamewere atomic, thenioutil.ReadFileshould work concurrently withrenameio.WriteFile.
What Rename are you talking about? I was talking about ioutil.ReadFile. ioutil.ReadFile uses Windows CreateFile with FILE_SHARE_READ | FILE_SHARE_WRITE, so any other program can write to the file during ioutil.ReadFile execution. And all new writes will be visible by ioutil.ReadFile as it read file byte by byte. That is whole point of FILE_SHARE_READ | FILE_SHARE_WRITE. If you want exclusive access to the file, you should not specify FILE_SHARE_READ | FILE_SHARE_WRITE.
It isn't obvious to me whether
ReplaceFileis atomic in that sense either. If it is not, then perhaps we will need to use the side-lock to guardgo.modandgo.sumedits on Windows. (We do not currently do any locking for those files, instead relying on the atomicity ofrenameiooperations.)
I did not look at your renameio, so I would not know. But you need file locks (or just drop FILE_SHARE_READ | FILE_SHARE_WRITE in CreateFile call) to make sure your file is getting read or written by only one program at a time.
Still you need to synchronize your file reads and writes, how do you propose to do that? You don't want your read happens before write. Also, even with file locks or without FILE_SHARE_READ | FILE_SHARE_WRITE, whoever comes second will get "Access denied" error:
h1 = CreateFile()
WriteFile(h1)
h2 = CreateFile()
CloseHandle(h1)
ReadFile(h2)
CloseHandle(h2)h2 = CreateFile() will fail with "Access denied" error
Alex
gopherbot
commented
5 years ago Change https://golang.org/cl/180517 mentions this issue: cmd/go: replace uses of ioutil.ReadFile with renameio.ReadFile
bcmills
commented
5 years ago I don't see how do you know that Windows MoveFileEx system call with the MOVEFILE_REPLACE_EXISTING flag is not atomic.
Empirical testing. 🙂
But empirical testing shows that ReplaceFile is also not atomic, so it looks like we're hosed either way.
ioutil.ReadFileuses Windows CreateFile with FILE_SHARE_READ | FILE_SHARE_WRITE, so any other program can write to the file duringioutil.ReadFileexecution.
Note that there is a third bit, FILE_SHARE_DELETE, that guards both deletion and renaming, which we do not currently set.
Unfortunately, even setting that bit does not seem to eliminate all of the ACCESS_DENIED or SHARING_VIOLATION errors.
If you want exclusive access to the file, you should not specify FILE_SHARE_READ | FILE_SHARE_WRITE.
I don't want exclusive access to the file — I want semantics consistent with POSIX, in which existing handles to the file see its existing contents from before the rename, handles opened after the rename see the new contents, and there is no fuzzy inaccessible state in the middle in which the file cannot be read at all.
you need file locks […] to make sure your file is getting read or written by only one program at a time.
In some cases, yes. Most of these operations follow the sequence read-prepare-lock-read-write-unlock, which works on all of the other supported platforms and minimizes the duration under which the file is locked. The problem with this test on Windows is that the first read operation fails.
alexbrainman
commented
5 years ago I don't want exclusive access to the file — I want semantics consistent with POSIX, in which existing handles to the file see its existing contents from before the rename, handles opened after the rename see the new contents, and there is no fuzzy inaccessible state in the middle in which the file cannot be read at all.
Sorry, but I still do not understand what you are talking about. You are saying rename, but I only see reading and writing of files. Perhaps if you could give an example in Go that runs on Linux, maybe I could try and make it work on Windows.
Thank you.
Alex
bcmills
commented
5 years ago Perhaps if you could give an example in Go that runs on Linux, maybe I could try and make it work on Windows.
That example is exactly the new test I added for the cmd/go/internal/renameio package. You can replicate the failure by removing the runtime.GOOS == "windows" special-cases from the test.
For now, the workaround of retrying both the read and rename operations on error seems to produce an acceptably low flake rate (for both renameio and TestScript/mod_concurrent). I haven't seen any flakes since those CLs landed.
alexbrainman
commented
5 years ago That example is exactly the new test I added for the
cmd/go/internal/renameiopackage. You can replicate the failure by removing theruntime.GOOS == "windows"special-cases from the test.
Thank you for the reference.
As far as understand your test just verifies that complete write of a file is read as a whole. Other than that there is no synchronization between readers or writers, or between writers, or between readers.
You could have achieved that by using CreateFile without either FILE_SHARE_READ or FILE_SHARE_WRITE. Then all file readers and writers would get exclusive access to the file, while others will get 'Access denied' (or similar) error. If you get 'Access denied' error, you would have to try again later.
You could also have used CreateFile with FILE_SHARE_READ. Similar to above, but all your readers can read file simultaneously. Other than similar to the above.
You could also use system sync objects (for example CreateMutexEx or similar) to control access to the file. These would allow you to wait for sync object to be released instead of trying in a loop.
For now, the workaround of retrying both the read and rename operations on error seems to produce an acceptably low flake rate (for both
renameioandTestScript/mod_concurrent). I haven't seen any flakes since those CLs landed.
Your approach uses os.Rename as atomic write operation. I do not know, if os.Rename behaves this way on Windows.
Alex
bcmills
commented
5 years ago As far as understand your test just verifies that complete write of a file is read as a whole. Other than that there is no synchronization between readers or writers, or between writers, or between readers.
Correct.
You could have achieved that by using CreateFile without either FILE_SHARE_READ or FILE_SHARE_WRITE. Then all file readers and writers would get exclusive access to the file, while others will get 'Access denied' (or similar) error. If you get 'Access denied' error, you would have to try again later.
My goal is for readers and writers not to need to retry at all. Concurrent writes are guarded by locking an external file, and concurrent reads should simply wait for the file to become available, then read it.
You could also use system sync objects (for example CreateMutexEx or similar) to control access to the file. These would allow you to wait for sync object to be released instead of trying in a loop.
We already use a (more portable) file lock to guard write access to files for which writes are important and not idempotent. In theory we could do the same for read access and for files for which dropped writes are OK (such as the cache-trimming timestamp) or for which writes are idempotent, but that would add needless locking overhead in by far the common case (concurrent readers with no writers).
We could add the locking only for Windows, but — especially given that our builders have much better coverage for Unix-like platforms — that seems like it would substantially increase the likelihood of introducing a subtle, Windows-only deadlock.
gopherbot
commented
5 years ago Change https://golang.org/cl/181541 mentions this issue: cmd/go: factor the I/O-retry logic out of renameio
This test is flaky on Windows:
https://storage.googleapis.com/go-build-log/bf2990d2/windows-amd64-2016_b2c5a5fb.log
/cc @jayconrod @bcmills