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
x/tools/gopls/internal/lsp/filecache: TestConcurrency failure on windows #57747
Comments
From the log and source, it's obviously a failure of MoveFileEx on Microsoft Windows. Seems like this is a fraught operation. Some digging led me to:
We might want to investigate ReplaceFile: |
(CC @golang/windows) From what I understand, guaranteed-atomic renames are not really a thing in the Win32 API. The caches in We also have |
filecache is already using robustio.Rename, so this issue is really just an indication that its probabilistic workaround isn't as reliable as we would like.
By idempotent write, I assume you mean that if you write a file whose name is
Thanks, I'll look into locking.
Yeah. It's sad that this still hasn't been addressed in the quarter century since I first hit this problem. It's not as if Windows exploits the lack of transactional renames to make its directory operations faster than POSIX---quite the opposite in fact, it is notoriously slow. (Some of that is no doubt due to hooks for Antivirus, backup, GUIs, search index updates, and other things that Linux doesn't support, of course.) Apparently, support for file system transactions (more general than just rename within a directory) was added to MS Windows Vista (2007) but has since been deprecated. |
No less an authority than the late Jim Gray of MSR says in To BLOB or Not To BLOB:
But I'm unable to reconcile that with these terrifying error codes documented for ReplaceFile:
Perhaps the operation is atomic within a directory, ACLs permitting, and not otherwise? |
It's complicated. 😅 Per https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-replacefilew#remarks,
From that description, I would suppose that if the file exists, it does have either the old contents or the new contents. However, if the two renames are not a single transaction then there may be an interval after the “rename the original file” step but before the “rename the new file” step in which the file ceases to exist entirely. 🤔 If that gap is possible, then it seems that simultaneous
So I would expect that |
Change https://go.dev/cl/461855 mentions this issue: |
Inspired by https://www.youtube.com/watch?v=uhRWMGBjlO8&t=2161s I tried an approach based on SetFileInformationByHandle(FileRenameInfo), but that doesn't seem to be any better. See https://go.dev/cl/461855. I notice that LLVM's solution to the same problem involved trying up to 200 retries (!!):
Sigh. |
Change https://go.dev/cl/461800 mentions this issue: |
Change https://go.dev/cl/463776 mentions this issue: |
The version of this file in the main repo uses a "!unix" build constraint, which had to be ported to a longer list for inclusion in gopls (which still supports Go versions that predate the "unix" tag). Solaris is a Unix derivative, and its implementation is provided in the "fcntl" implementation. Updates golang/go#57747. Change-Id: Ibde8ce55dadc03ad3cb797b5320f5b75580f639f Reviewed-on: https://go-review.googlesource.com/c/tools/+/463776 TryBot-Result: Gopher Robot <gobot@golang.org> Run-TryBot: Bryan Mills <bcmills@google.com> Reviewed-by: Robert Findley <rfindley@google.com> gopls-CI: kokoro <noreply+kokoro@google.com> Auto-Submit: Bryan Mills <bcmills@google.com>
Just noticed this failure on windows:
https://build.golang.org/log/bff6275bfcc6f47e62fef12d9b1607ddfe68a0ca
Could this be an artifact of the builder, or a problem with the filecache design on windows? We need to understand this, as the filecache is a critical new component of gopls.
CC @adonovan
The text was updated successfully, but these errors were encountered: