diff --git a/src/DodoSSH.Client.Shell/ViewModels/TransfersViewModel.cs b/src/DodoSSH.Client.Shell/ViewModels/TransfersViewModel.cs index a1f4078..cb00adf 100644 --- a/src/DodoSSH.Client.Shell/ViewModels/TransfersViewModel.cs +++ b/src/DodoSSH.Client.Shell/ViewModels/TransfersViewModel.cs @@ -250,6 +250,7 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo { private readonly ISftpSessionFactory sftp; private readonly FileTransferQueue queue; + private readonly Action post; private VaultViewModel? vault; private VaultKnownHostStore? knownHosts; @@ -269,9 +270,27 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo private bool disposed; - internal TransfersViewModel(ISftpSessionFactory sftp, TimeProvider clock) + /// Opens SFTP sessions. + /// Time source, for transfer rates and timestamps. + /// + /// Runs an action on the thread this view model's collections are read from. Defaults to the UI thread's + /// dispatcher, which is the answer in every real head. + /// + /// A delegate rather than Dispatcher.UIThread reached directly, for the reason + /// VaultViewModel's clipboard is one. Dispatcher.UIThread is process-wide and belongs to + /// whichever thread touched it first, so a test that posts through it is asserting on a queue owned by + /// some other test's thread — which passes or throws "the calling thread cannot access this object" + /// depending on the order a runner happened to schedule its classes in. Running the action inline + /// removes the thread from the question rather than making the test guess it right. + /// + /// + internal TransfersViewModel( + ISftpSessionFactory sftp, + TimeProvider clock, + Action? post = null) { this.sftp = sftp; + this.post = post ?? (action => Dispatcher.UIThread.Post(action)); // The supplier answers with whatever session is current at the moment a transfer starts, which is // what lets a queue survive a disconnect and reconnect without every queued row failing. @@ -1296,7 +1315,7 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo /// instead of one for the first change and one for the rest. /// private void OnTransferChanged(object? sender, TransferChangedEventArgs e) => - Dispatcher.UIThread.Post(() => + post(() => { if (Transfers.FirstOrDefault(row => row.Id == e.Transfer.Id) is { } existing) { diff --git a/tests/DodoSSH.Client.App.Tests/TransferQueueingTests.cs b/tests/DodoSSH.Client.App.Tests/TransferQueueingTests.cs index b9318e0..3882ee2 100644 --- a/tests/DodoSSH.Client.App.Tests/TransferQueueingTests.cs +++ b/tests/DodoSSH.Client.App.Tests/TransferQueueingTests.cs @@ -1,4 +1,4 @@ -using Avalonia.Threading; +using System.Collections.Concurrent; using DodoSSH.Client.Shell.ViewModels; using DodoSSH.Client.Ssh; using NSubstitute; @@ -26,10 +26,33 @@ public sealed class TransferQueueingTests : IDisposable private readonly string directory = Path.Combine(Path.GetTempPath(), $"dodossh-drop-{Guid.CreateVersion7():N}"); - private readonly TransfersViewModel transfers = - new(Substitute.For(), TimeProvider.System); + /// What the view model has posted and has not run yet. + /// + /// A queue of this test's own, standing in for the dispatcher — which is what it replaces. Draining + /// Dispatcher.UIThread meant depending on which thread a runner happened to touch that + /// process-wide object from first, and once another class got there first every test here died on + /// "the calling thread cannot access this object" having asserted nothing about transfers at all. + /// + /// A queue rather than a poster that runs the action inline, and the difference is not stylistic: the + /// transfer queue raises Changed from its pump thread as well as from the call that enqueued, + /// so inline execution would have a background thread adding rows to an ObservableCollection + /// while the test reads it. Draining keeps every mutation on the thread doing the asserting, which is + /// the one thing the dispatcher was providing that is worth keeping. + /// + /// + private readonly ConcurrentQueue posted = new(); - public TransferQueueingTests() => Directory.CreateDirectory(directory); + private readonly TransfersViewModel transfers; + + public TransferQueueingTests() + { + Directory.CreateDirectory(directory); + + transfers = new TransfersViewModel( + Substitute.For(), + TimeProvider.System, + posted.Enqueue); + } /// public void Dispose() @@ -162,17 +185,51 @@ public sealed class TransferQueueingTests : IDisposable transfers.Status.ShouldContain("1 folder was skipped"); } + /// + /// Queueing is reachable from any thread — a drop is handled on the UI thread, a retry is not — and + /// nothing about it may depend on which one. Worth stating because the version of this class that + /// drained Dispatcher.UIThread did depend on exactly that, and said so only by failing in CI on + /// a machine whose scheduling differed. The draining still happens on the test's own thread, as + /// explains; what is asserted here is the half that has no business caring. + /// + [Fact] + public void QueueingFromAnotherThread_StillEnqueues() + { + Exception? failure = null; + + var thread = new Thread(() => + { + try + { + Connected(); + transfers.QueueUploads([File("one.txt")]); + } + catch (Exception exception) + { + failure = exception; + } + }); + + thread.Start(); + thread.Join(); + + failure.ShouldBeNull(); + Queued().ShouldHaveSingleItem(); + } + /// The queue's rows, once the posts that create them have been let run. /// - /// TransfersViewModel adds a row from the queue's own Changed event, which it marshals - /// through Dispatcher.UIThread because the queue raises it from a pump thread. There is no - /// Avalonia application here to drain that, so the posts are run by hand — the alternative is asserting - /// on the status line alone, which is a string this code wrote about itself and proves nothing about - /// anything having been enqueued. + /// TransfersViewModel adds a row from the transfer queue's own Changed event, which it + /// marshals because the queue raises it from a pump thread. Nothing drains that here, so the posts are + /// run by hand — the alternative is asserting on the status line alone, which is a string this code + /// wrote about itself and proves nothing about anything having been enqueued. /// private IReadOnlyList Queued() { - Dispatcher.UIThread.RunJobs(); + while (posted.TryDequeue(out var action)) + { + action(); + } return transfers.Transfers; }