From 9f3dece0ab333d2f514bfb9e5663dcdd671f8ead Mon Sep 17 00:00:00 2001 From: Alan McGovern Date: Wed, 25 Jan 2023 23:22:12 +0000 Subject: [PATCH 1/2] [core] Fix several bugs with 'EngineSettings.UsePartialFiles' This adds several additional tests covering aspects of how this feature was intended to operate. The 'Complete' and 'Incomplete' paths are now generated using a uniform method. The choice of whether or not to use the 'Complete' or 'Incomplete' path as the current path is dependent on whether or not 'UsePartialFiles' is on, and if the file is complete. This should keep everything in sync when toggling UsePartialFiles on and off, and also if the user uses 'MoveFileAsync', or 'MoveFilesASync' to move files. --- .../MonoTorrent.Client/ClientEngine.cs | 8 +- .../Managers/DiskManager.cs | 23 +++--- .../Managers/TorrentFileInfo.cs | 23 +++++- .../Managers/TorrentManager.cs | 22 +++--- .../MonoTorrent.PieceWriter/DiskWriter.cs | 20 ++--- .../MonoTorrent.Client/ClientEngineTests.cs | 76 +++++++++++++++++++ .../DiskManagerExceptionTests.cs | 2 +- .../MonoTorrent.Client/DiskManagerTests.cs | 8 +- 8 files changed, 144 insertions(+), 38 deletions(-) diff --git a/src/MonoTorrent.Client/MonoTorrent.Client/ClientEngine.cs b/src/MonoTorrent.Client/MonoTorrent.Client/ClientEngine.cs index 985e9585b..6763b96e8 100644 --- a/src/MonoTorrent.Client/MonoTorrent.Client/ClientEngine.cs +++ b/src/MonoTorrent.Client/MonoTorrent.Client/ClientEngine.cs @@ -95,7 +95,11 @@ public static async Task RestoreStateAsync (ReadOnlyMemory b TorrentFileInfo torrentFile; torrentFile = (TorrentFileInfo) manager.Files.Single (t => t.Path == ((BEncodedString) file[nameof (torrentFile.Path)]).Text); torrentFile.Priority = (Priority) Enum.Parse (typeof (Priority), file[nameof (torrentFile.Priority)].ToString ()!); - torrentFile.FullPath = ((BEncodedString) file[nameof (torrentFile.FullPath)]).Text; + torrentFile.UpdatePaths (( + newPath: ((BEncodedString) file[nameof (torrentFile.FullPath)]).Text, + downloadCompletePath: ((BEncodedString) file[nameof (torrentFile.DownloadCompleteFullPath)]).Text, + downloadIncompletePath: ((BEncodedString) file[nameof (torrentFile.DownloadIncompleteFullPath)]).Text + )); } } else { var magnetLink = MagnetLink.Parse (torrent[nameof (manager.MagnetLink)].ToString ()!); @@ -127,6 +131,8 @@ public async Task SaveStateAsync () dict[nameof (t.Files)] = new BEncodedList (t.Files.Select (file => new BEncodedDictionary { { nameof(file.FullPath), (BEncodedString) file.FullPath }, + { nameof(file.DownloadCompleteFullPath), (BEncodedString) file.DownloadCompleteFullPath }, + { nameof(file.DownloadIncompleteFullPath), (BEncodedString) file.DownloadIncompleteFullPath }, { nameof(file.Path), (BEncodedString) file.Path }, { nameof(file.Priority), (BEncodedString) file.Priority.ToString () }, } diff --git a/src/MonoTorrent.Client/MonoTorrent.Client/Managers/DiskManager.cs b/src/MonoTorrent.Client/MonoTorrent.Client/Managers/DiskManager.cs index 6beb644e5..dc08c4c9e 100644 --- a/src/MonoTorrent.Client/MonoTorrent.Client/Managers/DiskManager.cs +++ b/src/MonoTorrent.Client/MonoTorrent.Client/Managers/DiskManager.cs @@ -455,27 +455,28 @@ public async Task FlushAsync (ITorrentManagerInfo manager, int startIndex, int e } } - internal Task MoveFileAsync (ITorrentManagerFile file, string newPath) - => MoveFileAsync ((TorrentFileInfo) file, newPath); + internal Task MoveFileAsync (ITorrentManagerFile file, (string newPath, string downloadComplete, string downloadIncomplete) paths) + => MoveFileAsync ((TorrentFileInfo) file, paths); - internal Task MoveFileAsync (TorrentFileInfo file, string newPath) - => MoveFileAsync (file, newPath, false); + internal Task MoveFileAsync (TorrentFileInfo file, (string newPath, string downloadComplete, string downloadIncomplete) paths) + => MoveFileAsync (file, paths, false); internal async Task MoveFilesAsync (IList files, string newRoot, bool overwrite) { - foreach (TorrentFileInfo file in files) - await MoveFileAsync (file, Path.Combine (newRoot, file.Path), overwrite); + foreach (TorrentFileInfo file in files) { + var paths = TorrentFileInfo.GetNewPaths (Path.GetFullPath (Path.Combine (newRoot, file.Path)), Settings.UsePartialFiles, file.Path == file.DownloadCompleteFullPath); + await MoveFileAsync (file, paths, overwrite); + } } - async Task MoveFileAsync (TorrentFileInfo file, string newPath, bool overwrite) + async Task MoveFileAsync (TorrentFileInfo file, (string newPath, string downloadCompletePath, string downloadIncompletePath) paths, bool overwrite) { await IOLoop; - newPath = Path.GetFullPath (newPath); - if (newPath != file.FullPath && await Cache.Writer.ExistsAsync (file)) { - await Cache.Writer.MoveAsync (file, newPath, false); + if (paths.newPath != file.FullPath && await Cache.Writer.ExistsAsync (file)) { + await Cache.Writer.MoveAsync (file, paths.newPath, false); } - file.FullPath = newPath; + file.UpdatePaths (paths); } internal async ReusableTask ReadAsync (ITorrentManagerInfo manager, BlockInfo request, Memory buffer) diff --git a/src/MonoTorrent.Client/MonoTorrent.Client/Managers/TorrentFileInfo.cs b/src/MonoTorrent.Client/MonoTorrent.Client/Managers/TorrentFileInfo.cs index 532b768ce..5c8beb0fb 100644 --- a/src/MonoTorrent.Client/MonoTorrent.Client/Managers/TorrentFileInfo.cs +++ b/src/MonoTorrent.Client/MonoTorrent.Client/Managers/TorrentFileInfo.cs @@ -29,6 +29,7 @@ using System; using System.Linq; +using System.Runtime; namespace MonoTorrent.Client { @@ -36,11 +37,11 @@ class TorrentFileInfo : ITorrentManagerFile { public static string IncompleteFileSuffix => ".!mt"; - public string DownloadCompleteFullPath { get; set; } + public string DownloadCompleteFullPath { get; private set; } - public string DownloadIncompleteFullPath { get; set; } + public string DownloadIncompleteFullPath { get; private set; } - public string FullPath { get; set; } + public string FullPath { get; private set; } ITorrentFile TorrentFile { get; } @@ -109,5 +110,21 @@ internal static string PathAndFileNameEscape (string path) filename = filename.Replace ($"{illegal}", $"_{Convert.ToString (illegal, 16)}_"); return System.IO.Path.Combine (dir, filename); } + + internal static (string path, string completePath, string incompletePath) GetNewPaths (string newPath, bool usePartialFiles, bool isComplete) + { + var downloadCompleteFullPath = newPath; + var downloadIncompleteFullPath = downloadCompleteFullPath + (usePartialFiles ? TorrentFileInfo.IncompleteFileSuffix : ""); + newPath = isComplete ? downloadCompleteFullPath : downloadIncompleteFullPath; + + return (newPath, downloadCompleteFullPath, downloadIncompleteFullPath); + } + + internal void UpdatePaths ((string newPath, string downloadCompletePath, string downloadIncompletePath) paths) + { + FullPath = paths.newPath; + DownloadCompleteFullPath = paths.downloadCompletePath; + DownloadIncompleteFullPath = paths.downloadIncompletePath; + } } } diff --git a/src/MonoTorrent.Client/MonoTorrent.Client/Managers/TorrentManager.cs b/src/MonoTorrent.Client/MonoTorrent.Client/Managers/TorrentManager.cs index e1341a377..94a352b3d 100644 --- a/src/MonoTorrent.Client/MonoTorrent.Client/Managers/TorrentManager.cs +++ b/src/MonoTorrent.Client/MonoTorrent.Client/Managers/TorrentManager.cs @@ -599,7 +599,8 @@ public async Task MoveFileAsync (ITorrentManagerFile file, string path) throw new TorrentException ("Cannot move files when the torrent is active"); try { - await Engine!.DiskManager.MoveFileAsync (file, path); + var paths = TorrentFileInfo.GetNewPaths (Path.GetFullPath (path), Engine.Settings.UsePartialFiles, file.Path == file.DownloadCompleteFullPath); + await Engine!.DiskManager.MoveFileAsync (file, paths); } catch (Exception ex) { TrySetError (Reason.WriteFailure, ex); throw; @@ -661,8 +662,11 @@ internal void SetMetadata (Torrent torrent) // All files marked as 'Normal' priority by default so 'PartialProgressSelector' // should be set to 'true' for each piece as all files are being downloaded. Files = Torrent.Files.Select (file => { - var downloadCompleteFullPath = Path.Combine (ContainingDirectory, TorrentFileInfo.PathAndFileNameEscape (file.Path)); - var downloadIncompleteFullPath = downloadCompleteFullPath + TorrentFileInfo.IncompleteFileSuffix; + + // Generate the paths when 'UsePartialFiles' is enabled. + var paths = TorrentFileInfo.GetNewPaths (Path.Combine (ContainingDirectory, TorrentFileInfo.PathAndFileNameEscape (file.Path)), usePartialFiles: true, isComplete: true); + var downloadCompleteFullPath = paths.completePath; + var downloadIncompleteFullPath = paths.incompletePath; // FIXME: Is this the best place to futz with actually moving files? if (!Engine!.Settings.UsePartialFiles) { @@ -673,10 +677,8 @@ internal void SetMetadata (Torrent torrent) } var currentPath = File.Exists (downloadCompleteFullPath) ? downloadCompleteFullPath : downloadIncompleteFullPath; - var torrentFileInfo = new TorrentFileInfo (file, currentPath) { - DownloadCompleteFullPath = downloadCompleteFullPath, - DownloadIncompleteFullPath = downloadIncompleteFullPath - }; + var torrentFileInfo = new TorrentFileInfo (file, currentPath); + torrentFileInfo.UpdatePaths ((currentPath, downloadCompleteFullPath, downloadIncompleteFullPath)); if (file.Length == 0) torrentFileInfo.BitField[0] = true; return torrentFileInfo; @@ -991,7 +993,7 @@ async void InvokePieceHashedAsync () internal async ReusableTask UpdateUsePartialFiles (bool usePartialFiles) { foreach (TorrentFileInfo file in Files) - file.DownloadIncompleteFullPath = file.DownloadCompleteFullPath + (usePartialFiles ? TorrentFileInfo.IncompleteFileSuffix : ""); + file.UpdatePaths ((file.FullPath, file.DownloadCompleteFullPath, file.DownloadCompleteFullPath + (usePartialFiles ? TorrentFileInfo.IncompleteFileSuffix : ""))); await RefreshPartialDownloadFilePaths (0, Files.Count); } @@ -1002,10 +1004,10 @@ internal async ReusableTask RefreshPartialDownloadFilePaths (int fileStartIndex, for (int i = fileStartIndex; i < fileStartIndex + count; i++) { if (files[i].BitField.AllTrue && files[i].FullPath != files[i].DownloadCompleteFullPath) { tasks ??= new List (); - tasks.Add (Engine!.DiskManager.MoveFileAsync (files[i], files[i].DownloadCompleteFullPath)); + tasks.Add (Engine!.DiskManager.MoveFileAsync (files[i], (files[i].DownloadCompleteFullPath, files[i].DownloadCompleteFullPath, files[i].DownloadIncompleteFullPath))); } else if (!files[i].BitField.AllTrue && files[i].FullPath != files[i].DownloadIncompleteFullPath) { tasks ??= new List (); - tasks.Add (Engine!.DiskManager.MoveFileAsync (files[i], files[i].DownloadIncompleteFullPath)); + tasks.Add (Engine!.DiskManager.MoveFileAsync (files[i], (files[i].DownloadIncompleteFullPath, files[i].DownloadCompleteFullPath, files[i].DownloadIncompleteFullPath))); } } if (tasks != null) diff --git a/src/MonoTorrent.PieceWriter/MonoTorrent.PieceWriter/DiskWriter.cs b/src/MonoTorrent.PieceWriter/MonoTorrent.PieceWriter/DiskWriter.cs index 1160b1ecb..9b3868cbc 100644 --- a/src/MonoTorrent.PieceWriter/MonoTorrent.PieceWriter/DiskWriter.cs +++ b/src/MonoTorrent.PieceWriter/MonoTorrent.PieceWriter/DiskWriter.cs @@ -142,17 +142,19 @@ public async ReusableTask MoveAsync (ITorrentManagerFile file, string newPath, b if (file is null) throw new ArgumentNullException (nameof (file)); - if (Streams.TryGetValue (file, out AllStreams? data)) { - using var releaser = await data.Locker.EnterAsync (); - await CloseAllAsync (data); + if (!Streams.TryGetValue (file, out AllStreams? data)) + Streams[file] = data = new AllStreams (); - if (File.Exists (file.FullPath)) { - if (overwrite) - File.Delete (newPath); + using var releaser = await data.Locker.EnterAsync (); + await CloseAllAsync (data).ConfigureAwait (false); - Directory.CreateDirectory (Path.GetDirectoryName (newPath)!); - File.Move (file.FullPath, newPath); - } + await new ThreadSwitcher (); + if (File.Exists (file.FullPath)) { + if (overwrite) + File.Delete (newPath); + + Directory.CreateDirectory (Path.GetDirectoryName (newPath)!); + File.Move (file.FullPath, newPath); } } diff --git a/src/Tests/Tests.MonoTorrent.Client/MonoTorrent.Client/ClientEngineTests.cs b/src/Tests/Tests.MonoTorrent.Client/MonoTorrent.Client/ClientEngineTests.cs index 4301c9c05..c5f5cdbca 100644 --- a/src/Tests/Tests.MonoTorrent.Client/MonoTorrent.Client/ClientEngineTests.cs +++ b/src/Tests/Tests.MonoTorrent.Client/MonoTorrent.Client/ClientEngineTests.cs @@ -191,6 +191,44 @@ public async Task ContainingDirectory_PathBusting () Assert.ThrowsAsync (() => rig.Engine.AddAsync (editor.ToTorrent (), "path", new TorrentSettings ())); } + [Test] + [TestCase (true)] + [TestCase (false)] + public async Task UsePartialFiles_InitiallyOff_ChangeFullPath_ToggleOn (bool createFile) + { + var pieceLength = Constants.BlockSize * 4; + var engine = new ClientEngine (EngineSettingsBuilder.CreateForTests (usePartialFiles: false)); + var torrent = TestRig.CreateMultiFileTorrent (TorrentFile.Create (pieceLength, Constants.BlockSize, Constants.BlockSize * 2, Constants.BlockSize * 3), pieceLength, out BEncoding.BEncodedDictionary _); + + using var tempDir = TempDir.Create (); + var manager = await engine.AddAsync (torrent, tempDir.Path); + Assert.AreEqual (manager.Files[0].DownloadCompleteFullPath, manager.Files[0].DownloadIncompleteFullPath); + + var newPath = Path.GetFullPath (Path.Combine (tempDir.Path, "new_full_path.fake")); + + await manager.MoveFileAsync (manager.Files[0], newPath); + Assert.AreEqual (newPath, manager.Files[0].FullPath); + Assert.AreEqual (newPath, manager.Files[0].DownloadCompleteFullPath); + Assert.AreEqual (newPath, manager.Files[0].DownloadIncompleteFullPath); + + if (createFile) + File.Create (manager.Files[0].FullPath).Dispose (); + + var settings = new EngineSettingsBuilder (engine.Settings) { UsePartialFiles = true }.ToSettings (); + await engine.UpdateSettingsAsync (settings); + Assert.AreEqual (newPath + TorrentFileInfo.IncompleteFileSuffix, manager.Files[0].FullPath); + Assert.AreEqual (newPath, manager.Files[0].DownloadCompleteFullPath); + Assert.AreEqual (newPath + TorrentFileInfo.IncompleteFileSuffix, manager.Files[0].DownloadIncompleteFullPath); + + if (createFile) { + Assert.IsFalse (File.Exists (newPath)); + Assert.IsTrue (File.Exists (newPath + TorrentFileInfo.IncompleteFileSuffix)); + } else { + Assert.IsFalse (File.Exists (newPath)); + Assert.IsFalse (File.Exists (newPath + TorrentFileInfo.IncompleteFileSuffix)); + } + } + [Test] public async Task UsePartialFiles_InitiallyOff_ToggleOn () { @@ -221,6 +259,44 @@ public async Task UsePartialFiles_InitiallyOn_ToggleOff () Assert.AreEqual (manager.Files[0].DownloadCompleteFullPath, manager.Files[0].DownloadIncompleteFullPath); } + [Test] + [TestCase (true)] + [TestCase (false)] + public async Task UsePartialFiles_InitiallyOn_ChangeFullPath_ToggleOff (bool createFile) + { + var pieceLength = Constants.BlockSize * 4; + var engine = new ClientEngine (EngineSettingsBuilder.CreateForTests (usePartialFiles: true)); + var torrent = TestRig.CreateMultiFileTorrent (TorrentFile.Create (pieceLength, Constants.BlockSize, Constants.BlockSize * 2, Constants.BlockSize * 3), pieceLength, out BEncoding.BEncodedDictionary _); + + using var tempDir = TempDir.Create (); + var manager = await engine.AddAsync (torrent, tempDir.Path); + Assert.AreNotEqual (manager.Files[0].DownloadCompleteFullPath, manager.Files[0].DownloadIncompleteFullPath); + + var newPath = Path.GetFullPath (Path.Combine (tempDir.Path, "new_full_path.fake")); + + await manager.MoveFileAsync (manager.Files[0], newPath); + Assert.AreEqual (newPath + TorrentFileInfo.IncompleteFileSuffix, manager.Files[0].FullPath); + Assert.AreEqual (newPath, manager.Files[0].DownloadCompleteFullPath); + Assert.AreEqual (newPath + TorrentFileInfo.IncompleteFileSuffix, manager.Files[0].DownloadIncompleteFullPath); + + if (createFile) + File.Create (manager.Files[0].FullPath).Dispose (); + + var settings = new EngineSettingsBuilder (engine.Settings) { UsePartialFiles = false }.ToSettings (); + await engine.UpdateSettingsAsync (settings); + Assert.AreEqual (newPath, manager.Files[0].FullPath); + Assert.AreEqual (newPath, manager.Files[0].DownloadCompleteFullPath); + Assert.AreEqual (newPath, manager.Files[0].DownloadIncompleteFullPath); + + if (createFile) { + Assert.IsFalse (File.Exists (newPath + TorrentFileInfo.IncompleteFileSuffix)); + Assert.IsTrue (File.Exists (newPath)); + } else { + Assert.IsFalse (File.Exists (newPath)); + Assert.IsFalse (File.Exists (newPath + TorrentFileInfo.IncompleteFileSuffix)); + } + } + [Test] public void DownloadMetadata_Cancelled () { diff --git a/src/Tests/Tests.MonoTorrent.Client/MonoTorrent.Client/DiskManagerExceptionTests.cs b/src/Tests/Tests.MonoTorrent.Client/MonoTorrent.Client/DiskManagerExceptionTests.cs index 68f261a7c..f30bd62cb 100644 --- a/src/Tests/Tests.MonoTorrent.Client/MonoTorrent.Client/DiskManagerExceptionTests.cs +++ b/src/Tests/Tests.MonoTorrent.Client/MonoTorrent.Client/DiskManagerExceptionTests.cs @@ -157,7 +157,7 @@ public void CloseFail () public void MoveFileFail () { writer.move = true; - Assert.ThrowsAsync (() => diskManager.MoveFileAsync ((TorrentFileInfo) data.Files[0], "root")); + Assert.ThrowsAsync (() => diskManager.MoveFileAsync ((TorrentFileInfo) data.Files[0], ("root", "bar", "baz"))); } [Test] diff --git a/src/Tests/Tests.MonoTorrent.Client/MonoTorrent.Client/DiskManagerTests.cs b/src/Tests/Tests.MonoTorrent.Client/MonoTorrent.Client/DiskManagerTests.cs index 95e981da0..c22aaba49 100644 --- a/src/Tests/Tests.MonoTorrent.Client/MonoTorrent.Client/DiskManagerTests.cs +++ b/src/Tests/Tests.MonoTorrent.Client/MonoTorrent.Client/DiskManagerTests.cs @@ -338,8 +338,10 @@ public async Task MoveFile_ConvertsToFullPath () var file = TorrentFileInfo.Create (Constants.BlockSize, 123456).Single (); Assert.IsFalse (File.Exists (file.FullPath)); - await manager.MoveFileAsync (file, "NewPath"); + await manager.MoveFileAsync (file, ("NewFullPath", "NewFullPath", "NewIncompletePath")); Assert.AreEqual (Path.GetFullPath ("NewPath"), file.FullPath); + Assert.AreEqual (Path.GetFullPath ("NewPath"), file.DownloadCompleteFullPath); + Assert.AreEqual (Path.GetFullPath ("NewPath"), file.DownloadIncompleteFullPath); Assert.IsFalse (File.Exists (file.FullPath)); } @@ -353,7 +355,7 @@ public async Task MoveFile_SamePath () using var writer = new TestPieceWriter (); using var manager = new DiskManager (new EngineSettings (), Factories.Default, writer); - await manager.MoveFileAsync (file, file.FullPath); + await manager.MoveFileAsync (file, (file.FullPath, file.FullPath, file.FullPath + TorrentFileInfo.IncompleteFileSuffix)); Assert.IsTrue (File.Exists (file.FullPath)); } @@ -368,7 +370,7 @@ public async Task MoveFile_TargetDirectoryDoesNotExist () using var manager = new DiskManager (new EngineSettings (), Factories.Default, writer); var fullPath = Path.Combine (tmp.Path, "New", "Path", "file.txt"); - await manager.MoveFileAsync (file, fullPath); + await manager.MoveFileAsync (file, (fullPath, fullPath, fullPath + TorrentFileInfo.IncompleteFileSuffix)); Assert.AreEqual (fullPath, file.FullPath); } From 3daf4ef38672d52f4413392bf2114cc45954e0a7 Mon Sep 17 00:00:00 2001 From: Alan McGovern Date: Fri, 27 Jan 2023 19:12:54 +0000 Subject: [PATCH 2/2] Fix the old test --- .../MonoTorrent.Client/DiskManagerTests.cs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/Tests/Tests.MonoTorrent.Client/MonoTorrent.Client/DiskManagerTests.cs b/src/Tests/Tests.MonoTorrent.Client/MonoTorrent.Client/DiskManagerTests.cs index c22aaba49..261b433c1 100644 --- a/src/Tests/Tests.MonoTorrent.Client/MonoTorrent.Client/DiskManagerTests.cs +++ b/src/Tests/Tests.MonoTorrent.Client/MonoTorrent.Client/DiskManagerTests.cs @@ -338,10 +338,11 @@ public async Task MoveFile_ConvertsToFullPath () var file = TorrentFileInfo.Create (Constants.BlockSize, 123456).Single (); Assert.IsFalse (File.Exists (file.FullPath)); - await manager.MoveFileAsync (file, ("NewFullPath", "NewFullPath", "NewIncompletePath")); - Assert.AreEqual (Path.GetFullPath ("NewPath"), file.FullPath); - Assert.AreEqual (Path.GetFullPath ("NewPath"), file.DownloadCompleteFullPath); - Assert.AreEqual (Path.GetFullPath ("NewPath"), file.DownloadIncompleteFullPath); + var newFullPath = Path.GetFullPath ("NewFullPath"); + await manager.MoveFileAsync (file, (newFullPath, newFullPath, newFullPath)); + Assert.AreEqual (newFullPath, file.FullPath); + Assert.AreEqual (newFullPath, file.DownloadCompleteFullPath); + Assert.AreEqual (newFullPath, file.DownloadIncompleteFullPath); Assert.IsFalse (File.Exists (file.FullPath)); }