Fix unsafe file writes in multiple framework components #5178
Conversation
As noticed in ppy#5177, writes of these files have been unsafe until now if the file write is interrupted partway. The length of the written file could be shorter than expected. This uses a newly added method to write to a temporary file before moving to the final location.
|
I've updated osu-side usages to show how many places this can be used in. I've left a few which are responsible for creating backup files ( |
|
Ignoring the test failures (investigating), one consideration we may want to address is deleting the temporary file when we can. ie. if we get to the dispose call and are aware of an exception. Haven't yet checked whether this is possible or not. |
osu.Framework/Platform/Storage.cs
Outdated
| /// If the target file path already exists, it will be deleted before attempting to write a new version. | ||
| /// </remarks> | ||
| /// <param name="path">The path of the file to create or overwrite.</param> | ||
| /// <returns></returns> |
There was a problem hiding this comment.
| /// <returns></returns> | |
| /// <returns>A <see cref="Stream"/> to which contents of the file to be created safely should be written.</returns> |
or just remove, idunno - bit hard to explain that one
| public SafeWriteStream(string temporaryPath, string finalPath, Storage storage) | ||
| : base(storage.GetFullPath(temporaryPath, true), FileMode.Create, FileAccess.Write) | ||
| { | ||
| storage.Delete(finalPath); |
There was a problem hiding this comment.
Out of curiosity mostly, isn't this delete kind of useless? Like to avoid any plausible concurrency issues, I'd say that I'd imagine it should happen as close to the move operation as possible, i.e. in Dispose()? This is the scenario that I'm thinking of here:
Thread A Thread B
streamA = new SafeFileStream("tempA.txt", "file.txt", storage)
storage.Delete("file.txt")
streamB = new SafeFileStream("tempB.txt", "file.txt", storage)
storage.Delete("file.txt")
streamB.Dispose()
storage.Move("tempB.txt", "file.txt")
streamA.Dispose()
storage.Move("tempA.txt", "file.txt")
The file delete doesn't really do anything useful for this sort of scenario. Moving it to disposal wouldn't eliminate the race there, but it would make it less likely to happen.
There was a problem hiding this comment.
I was more thinking about the case where the stream write operation failed along the way, in which case doing an early delete will potentially clean up an old version of the file (and leave no file instead).
A bit of an undefined expectation, and I'm not sure how correct it is.
There was a problem hiding this comment.
@smoogipoo interested in your opinion on this one
There was a problem hiding this comment.
If we're talking a power outage or force-quit of the application, then this method of doing things is very dangerous. Because in that scenario you have no way of recovery - the original file has been deleted and the new file has potentially not finished writing yet.
How about enforcing a non-random temporary path such as _{filename}, and always delete that path instead?
There was a problem hiding this comment.
Are you suggesting renaming the original file to a temporary path before starting the write operation? ie. allowing for manual recovery?
There was a problem hiding this comment.
No, I'm suggesting writing to a fixed temporary path instead.
File.Delete("_{filename}");
// Write to _{filename}
File.Delete("{filename}");
File.Move("_{filename}", "{filename}");
Or, as I recently found out, File.Replace() is a thing which does a backup. So something like:
File.Delete("_{filename}");
// Write to _{filename}
File.Replace("_{filename}", "{filename}", "{filename}_backup");
File.Delete("{filename}_backup");
You could even timestamp the backup similar to what's done in some areas of osu! to ensure there's always a backup.
There was a problem hiding this comment.
First of all, the File.Delete you're proposing isn't required because we are using Create which overwrites.
But it's safe to say you conceptually disagree with "early clean up" as mentioned in #5178 (comment)? ie. leaving the old file in place until the new write is 100% completed is what we want?
I think I agree that this is better behaviour. I originally had the clean-up early as it was removing broken cache files, but now that we are using this in more places it makes less sense.
There was a problem hiding this comment.
Oh, and if File.Replace is doing a Copy operation to create the backup, it can't be used here. As that will add a performance overhead I was looking to avoid by only using Move operations.
There was a problem hiding this comment.
But it's safe to say you conceptually disagree with "early clean up" as mentioned in #5178 (comment)? ie. leaving the old file in place until the new write is 100% completed is what we want?
Correct.
As that will add a performance overhead
I'm actually curious on whether it would copy (in particular on Windows). See this comment on LinkOrCopyFile:
There was a problem hiding this comment.
Probably using hard links internally (what I also want to use eventually for beatmap importing). Maybe okay.
|
Looks to be failing tests. |
As noticed in #5177, writes of these files have been unsafe until now if the file write is interrupted partway. The length of the written file could be shorter than expected.
This uses a newly added method to write to a temporary file before moving to the final location.
osu-side branch for testing: https://github.com/ppy/osu/compare/master...peppy:update-framework?expand=1
Closes #5177.