-
Notifications
You must be signed in to change notification settings - Fork 508
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
Fix windows race condition when writing image with duplicate layers #1921
Fix windows race condition when writing image with duplicate layers #1921
Conversation
Signed-off-by: David Gannon <19214156+dgannon991@users.noreply.github.com>
I'd say a more robust solution is "try to rename, if it fails, but destination exists, just remove the source and pretend you're fine". This would also cover the case when multiple |
I had a very similar issue in git-lfs in the past. Here's the analysis: git-lfs/git-lfs#3880 (comment) and here's the final fix that solved the issue: git-lfs/git-lfs@662a624 |
I'll give that a go, thank you for the feedback. |
@slonopotamus I've written this up and given it a test, and I can confirm it works: err = os.Rename(w.Name(), renamePath)
var badInputErr *os.LinkError
if err != nil && errors.As(err, &badInputErr) && badInputErr.Err == syscall.ERROR_ACCESS_DENIED {
if _, err := os.Stat(renamePath); err == nil {
// windows os.Rename() issue #1920
// the destination already exists, just remove the target and continue
return os.Remove(w.Name())
}
}
return err I do have to say though of the two solutions I think I'm leaning towards the first one, as it makes os.Rename behave more like linux (in that it's an atomic transaction.) Would you mind if I left this up for a second opinion? I'm more than happy to swap over to this approach, but I worry it would mask other issues outside of the scope of this ticket (as you mentioned, running two crane processes at the same time, it got me thinking that realistically that /should/ be an error!) |
Sure, I'm just passing by, and simply gave my thoughts. |
@slonopotamus - thank you for taking the time, the thoughts were definitely appreciated, and it was great to have a confirm that it was an issue in other places! |
Hi Guys, |
This is a potential solution for #1920, any feedback would be greatly appreciated.