fix: use a tempfile to write files in filestorage. #300
Conversation
|
Thanks for the PR! I don't understand much about this (you have been very helpful explaining it in Slack so far, which I appreciate) -- is this change ready to go? Does it work for all the platforms it is supposed to work on (and it doesn't break on others)? I will give it a closer look ASAP :) |
|
yes this is ready. it should work fine and shouldn't break anything on windows. |
| _, err = fp.Write(value) | ||
| if err != nil { | ||
| // cancel the write | ||
| fp.Cancel() |
There was a problem hiding this comment.
Why doesn't the fp automatically cancel the write when there's an error?
There was a problem hiding this comment.
cancel is a special call which deletes the temp file.
the fp to the tempfile can't know to delete itself when it errors, so we tell it to
|
One more quick question -- how do you think this will work with NFS mounts? We have some people using it with NFS, though I don't recommend it because I know NFS already has some issues related to O_EXCL and possibly other things; do you think this will make the problem worse or better? (Or neither) |
it should be better or the same. NFS mounts have more lag, they are maybe more likely to run into the race condition that this fixes. but also, NFS has so many issues for stuff like this that I can't say for sure it will anything better. I don't know if this will fix issues with o_excl. I don't use it and it's fundamentally broken on NFS so I doubt it will. |
elee1766
force-pushed
the
write-race-fix
branch
from
5cfa650
to
f00a187
Compare
| @@ -0,0 +1,3 @@ | |||
| # testutil | |||
|
|
|||
| some testing functions copied out of github.com/stretchr/testify, which were originally used by atomicfile | |||
There was a problem hiding this comment.
This is fine -- but it's MIT licensed code so we should probably copy and paste the license (which contains the copyright) into this readme file, just in case.
discovered in #296
there is a currently a race between file deletion and rewrite which can cause an empty read.
copying the method used by containerd, https://github.com/containerd/containerd/blob/main/pkg%2Fatomicfile%2Ffile.go the problem can be solved via a flushing a temporary file