safe writer #60
safe writer #60
Conversation
|
soooo how much would people crucify me if i used |
|
Big thing that's missing now is that while the tests hit all the happy paths (based on looking at coverage), they don't hit any of the failure, or fail+rollback cases that occur. Maybe could force such failures with some creative chmodding... |
|
and now, using the new writer for all the existing commands. woohoo! |
|
Discussed a bit with @sdboyer yesterday and while I think there are usable bits here, I think it may be a little too defensive. For instance, most errors that will happen during file operations will be IO/file errors and once you hit one of those, it's likely that later ones on the same device(s) / filesystem(s) will also fail. So I prefer the write to temp and fail early bits if possible. |
|
what if i refactored to make it attempt a rollback (if one's needed), but if the rollback fails then just stop all further rollback attempts, and inform the user about the current state of disk as best we can? |
| // renameWithFallback attempts to rename a file or directory, but falls back to | ||
| // copying in the event of a cross-link device error. If the fallback copy | ||
| // succeeds, src is still removed, emulating normal rename behavior. | ||
| func renameWithFallback(src, dest string) error { |
There was a problem hiding this comment.
I'm unsure what the gain is here. In go 1.8 we can't rename a dir on linux either. I actually got the cross link error which is why we just defaulted to copying everything lol, but I almost feel like having less code and defaulting to just copy is not that bad, idk
There was a problem hiding this comment.
In go 1.8 we can't rename a dir on linux either.
i'm confused - is that this change, where renaming to an empty dir will now fail, whereas it succeeded before?
I'm unsure what the gain is here.
I think it varies from case to case, but in general, the gain is just that a move, barring the cross-link errors, is pretty much instantaneous. Whereas copying, especially if it's a large tree...nossomuch.
The only real benefit of having this func is that it we get superfast-ness by default (in situations where renames are appropriate), but if that fails, it falls back to good ol' copying. So we get rename-like behavior either way, and don't have to sweat the details of whether it did so with renaming or copying.
and defaulting to just copy is not that bad, idk
The only place this PR used this func is in the safewriter. It uses it for everything - manifest, lock, and vendor - on the possibility that the temp directory is on a different device than the project directory. For the first two, it doesn't really matter. But, for vendor...well, if you've got e.g. k8s in there, then that's potentially quite a lot of wall time spent copying. That seems worth this extra logic, to me.
There was a problem hiding this comment.
I guess one of our questions was, why all of the assertions? Just try falling backing to a copy if there is an error on os.Rename and return any errors from the copy.
There was a problem hiding this comment.
i...literally never thought about that. lol. this code evolved out of glide's rename mechanism, where people were complaining about cross-link errors, so addressing that was the specific goal.
the only only thing that would worry me about opening it up more is that i don't really feel like i have a good handle on all the different possible and crazy things that filesystems can error with (being that files are, as a general rule, terrible). so, i don't know if we'd be unintentionally allowing for some icky bugs if we did that.
a concrete example might be the aforementioned os.Rename change with dirs on linux - is it advisable to attempt a copy when the rename error indicates the dir exists, but is empty? my first inclination is "probably," but i wonder how many more cases like this there might be.
|
We're going to rebase and merge this, but I think it's ripe for a refactor at some point later on. |
Simple helper func to see if two *lock instances are equivalent - same memo, same projects.
Very much incomplete, in the process of being adapted from the one I already wrote for glide.
Still frustratingly incomplete, as we're not actually testing the rollback cases which the transaction writer is really supposed to facilitate.
Changes both name and implementation of renameElseCopy to reflect that the outcome will be like a rename, even in the event of a cross-device link error.
Signed-off-by: Jess Frazelle <[email protected]>
@freeformz agree, i'm gonna open an issue or two about it now |
Prior to this commit, there were two (maybe one) places where solver behavior could be made non-deterministic due to the randomness of map iteration. The first is when populating the atomWithPackages representing the root package. PackageTree.Packages was ranged over and the keys used to construct a []string, which was then saved as package list on the awp. This probably wasn't causing non-determinism, but it was an easy fix, and a one-time (root-only) sort, so no reason not to fix it. The other case was walking a map of needed external packages in getImportsAndConstraintsOf(), which ultimately determined the order in which an atom's dependencies were checked against the satisfiability conditions. This most certainly affected solver determinism. Fixes golang#60.
Fixes #19