From 4bd685576446df941e9fa17e5b09b0068082b6bb Mon Sep 17 00:00:00 2001 From: Spike Curtis Date: Thu, 17 Apr 2025 16:02:43 +0400 Subject: [PATCH 1/3] fix: fix downloading different URLs to same destination --- Tests.Vpn.Service/DownloaderTest.cs | 34 ++++++++++++++++++++++++++--- Vpn.Service/Downloader.cs | 23 ++++++++++--------- 2 files changed, 42 insertions(+), 15 deletions(-) diff --git a/Tests.Vpn.Service/DownloaderTest.cs b/Tests.Vpn.Service/DownloaderTest.cs index 985e331..fd0ca58 100644 --- a/Tests.Vpn.Service/DownloaderTest.cs +++ b/Tests.Vpn.Service/DownloaderTest.cs @@ -284,6 +284,34 @@ public async Task Download(CancellationToken ct) Assert.That(await File.ReadAllTextAsync(destPath, ct), Is.EqualTo("test")); } + [Test(Description = "Perform 2 downloads with the same destination")] + [CancelAfter(30_000)] + public async Task DownloadSameDest(CancellationToken ct) + { + using var httpServer = EchoServer(); + var url0 = new Uri(httpServer.BaseUrl + "/test0"); + var url1 = new Uri(httpServer.BaseUrl + "/test1"); + var destPath = Path.Combine(_tempDir, "test"); + + var manager = new Downloader(NullLogger.Instance); + var startTask0 = manager.StartDownloadAsync(new HttpRequestMessage(HttpMethod.Get, url0), destPath, + NullDownloadValidator.Instance, ct); + var startTask1 = manager.StartDownloadAsync(new HttpRequestMessage(HttpMethod.Get, url1), destPath, + NullDownloadValidator.Instance, ct); + var dlTask0 = await startTask0; + await dlTask0.Task; + Assert.That(dlTask0.TotalBytes, Is.EqualTo(5)); + Assert.That(dlTask0.BytesRead, Is.EqualTo(5)); + Assert.That(dlTask0.Progress, Is.EqualTo(1)); + Assert.That(dlTask0.IsCompleted, Is.True); + var dlTask1 = await startTask1; + await dlTask1.Task; + Assert.That(dlTask1.TotalBytes, Is.EqualTo(5)); + Assert.That(dlTask1.BytesRead, Is.EqualTo(5)); + Assert.That(dlTask1.Progress, Is.EqualTo(1)); + Assert.That(dlTask1.IsCompleted, Is.True); + } + [Test(Description = "Download with custom headers")] [CancelAfter(30_000)] public async Task WithHeaders(CancellationToken ct) @@ -395,9 +423,9 @@ public void CancelledOuter(CancellationToken ct) var manager = new Downloader(NullLogger.Instance); // The "outer" Task should fail. var smallerCt = new CancellationTokenSource(TimeSpan.FromSeconds(1)).Token; - Assert.ThrowsAsync( - async () => await manager.StartDownloadAsync(new HttpRequestMessage(HttpMethod.Get, url), destPath, - NullDownloadValidator.Instance, smallerCt)); + Assert.ThrowsAsync(async () => await manager.StartDownloadAsync( + new HttpRequestMessage(HttpMethod.Get, url), destPath, + NullDownloadValidator.Instance, smallerCt)); } [Test(Description = "Timeout on response body")] diff --git a/Vpn.Service/Downloader.cs b/Vpn.Service/Downloader.cs index a37a1ec..c6ebb69 100644 --- a/Vpn.Service/Downloader.cs +++ b/Vpn.Service/Downloader.cs @@ -288,7 +288,9 @@ public async Task StartDownloadAsync(HttpRequestMessage req, strin { var task = _downloads.GetOrAdd(destinationPath, _ => new DownloadTask(_logger, req, destinationPath, validator)); - await task.EnsureStartedAsync(ct); + // EnsureStarted is a no-op if we didn't create a new DownloadTask. + // So, we will only remove the destination once for each time we start a new task. + task.EnsureStarted(tsk => _downloads.TryRemove(destinationPath, out _), ct); // If the existing (or new) task is for the same URL, return it. if (task.Request.RequestUri == req.RequestUri) @@ -357,13 +359,11 @@ internal DownloadTask(ILogger logger, HttpRequestMessage req, string destination ".download-" + Path.GetRandomFileName()); } - internal async Task EnsureStartedAsync(CancellationToken ct = default) + internal void EnsureStarted(Action continuation, CancellationToken ct = default) { - using var _ = await _semaphore.LockAsync(ct); + using var _ = _semaphore.Lock(); if (Task == null!) - Task = await StartDownloadAsync(ct); - - return Task; + Task = Start(ct).ContinueWith(continuation, CancellationToken.None); } /// @@ -371,7 +371,7 @@ internal async Task EnsureStartedAsync(CancellationToken ct = default) /// and the download will continue in the background. The provided CancellationToken can be used to cancel the /// download. /// - private async Task StartDownloadAsync(CancellationToken ct = default) + private async Task Start(CancellationToken ct = default) { Directory.CreateDirectory(_destinationDirectory); @@ -398,8 +398,7 @@ private async Task StartDownloadAsync(CancellationToken ct = default) throw new Exception("Existing file failed validation after 304 Not Modified", e); } - Task = Task.CompletedTask; - return Task; + return; } if (res.StatusCode != HttpStatusCode.OK) @@ -432,11 +431,11 @@ private async Task StartDownloadAsync(CancellationToken ct = default) throw; } - Task = DownloadAsync(res, tempFile, ct); - return Task; + await Download(res, tempFile, ct); + return; } - private async Task DownloadAsync(HttpResponseMessage res, FileStream tempFile, CancellationToken ct) + private async Task Download(HttpResponseMessage res, FileStream tempFile, CancellationToken ct) { try { From 35abc67660953308924658aa83bcc962202ec347 Mon Sep 17 00:00:00 2001 From: Spike Curtis Date: Thu, 17 Apr 2025 17:13:17 +0400 Subject: [PATCH 2/3] handle exceptions and fix UTs --- Tests.Vpn.Service/DownloaderTest.cs | 34 +++++++---------------------- Vpn.Service/Downloader.cs | 22 +++++++++++++++++-- 2 files changed, 28 insertions(+), 28 deletions(-) diff --git a/Tests.Vpn.Service/DownloaderTest.cs b/Tests.Vpn.Service/DownloaderTest.cs index fd0ca58..8bfa367 100644 --- a/Tests.Vpn.Service/DownloaderTest.cs +++ b/Tests.Vpn.Service/DownloaderTest.cs @@ -375,17 +375,17 @@ public async Task DownloadExistingDifferentContent(CancellationToken ct) [Test(Description = "Unexpected response code from server")] [CancelAfter(30_000)] - public void UnexpectedResponseCode(CancellationToken ct) + public async Task UnexpectedResponseCode(CancellationToken ct) { using var httpServer = new TestHttpServer(ctx => { ctx.Response.StatusCode = 404; }); var url = new Uri(httpServer.BaseUrl + "/test"); var destPath = Path.Combine(_tempDir, "test"); var manager = new Downloader(NullLogger.Instance); - // The "outer" Task should fail. - var ex = Assert.ThrowsAsync(async () => - await manager.StartDownloadAsync(new HttpRequestMessage(HttpMethod.Get, url), destPath, - NullDownloadValidator.Instance, ct)); + // The "inner" Task should fail. + var dlTask = await manager.StartDownloadAsync(new HttpRequestMessage(HttpMethod.Get, url), destPath, + NullDownloadValidator.Instance, ct); + var ex = Assert.ThrowsAsync(async () => await dlTask.Task); Assert.That(ex.Message, Does.Contain("404")); } @@ -412,22 +412,6 @@ public async Task MismatchedETag(CancellationToken ct) Assert.That(ex.Message, Does.Contain("ETag does not match SHA1 hash of downloaded file").And.Contains("beef")); } - [Test(Description = "Timeout on response headers")] - [CancelAfter(30_000)] - public void CancelledOuter(CancellationToken ct) - { - using var httpServer = new TestHttpServer(async _ => { await Task.Delay(TimeSpan.FromSeconds(5), ct); }); - var url = new Uri(httpServer.BaseUrl + "/test"); - var destPath = Path.Combine(_tempDir, "test"); - - var manager = new Downloader(NullLogger.Instance); - // The "outer" Task should fail. - var smallerCt = new CancellationTokenSource(TimeSpan.FromSeconds(1)).Token; - Assert.ThrowsAsync(async () => await manager.StartDownloadAsync( - new HttpRequestMessage(HttpMethod.Get, url), destPath, - NullDownloadValidator.Instance, smallerCt)); - } - [Test(Description = "Timeout on response body")] [CancelAfter(30_000)] public async Task CancelledInner(CancellationToken ct) @@ -479,12 +463,10 @@ public async Task ValidationFailureExistingFile(CancellationToken ct) await File.WriteAllTextAsync(destPath, "test", ct); var manager = new Downloader(NullLogger.Instance); + var dlTask = await manager.StartDownloadAsync(new HttpRequestMessage(HttpMethod.Get, url), destPath, + new TestDownloadValidator(new Exception("test exception")), ct); // The "outer" Task should fail because the inner task never starts. - var ex = Assert.ThrowsAsync(async () => - { - await manager.StartDownloadAsync(new HttpRequestMessage(HttpMethod.Get, url), destPath, - new TestDownloadValidator(new Exception("test exception")), ct); - }); + var ex = Assert.ThrowsAsync(async () => { await dlTask.Task; }); Assert.That(ex.Message, Does.Contain("Existing file failed validation")); Assert.That(ex.InnerException, Is.Not.Null); Assert.That(ex.InnerException!.Message, Is.EqualTo("test exception")); diff --git a/Vpn.Service/Downloader.cs b/Vpn.Service/Downloader.cs index c6ebb69..6a665ae 100644 --- a/Vpn.Service/Downloader.cs +++ b/Vpn.Service/Downloader.cs @@ -3,6 +3,7 @@ using System.Formats.Asn1; using System.Net; using System.Runtime.CompilerServices; +using System.Runtime.ExceptionServices; using System.Security.Cryptography; using System.Security.Cryptography.X509Certificates; using Coder.Desktop.Vpn.Utilities; @@ -290,7 +291,24 @@ public async Task StartDownloadAsync(HttpRequestMessage req, strin _ => new DownloadTask(_logger, req, destinationPath, validator)); // EnsureStarted is a no-op if we didn't create a new DownloadTask. // So, we will only remove the destination once for each time we start a new task. - task.EnsureStarted(tsk => _downloads.TryRemove(destinationPath, out _), ct); + task.EnsureStarted(tsk => + { + // remove the key first, before checking the exception, to ensure + // we still clean up. + _downloads.TryRemove(destinationPath, out _); + if (tsk.Exception == null) + { + return; + } + + if (tsk.Exception.InnerException != null) + { + ExceptionDispatchInfo.Capture(tsk.Exception.InnerException).Throw(); + } + + // not sure if this is hittable, but just in case: + throw tsk.Exception; + }, ct); // If the existing (or new) task is for the same URL, return it. if (task.Request.RequestUri == req.RequestUri) @@ -363,7 +381,7 @@ internal void EnsureStarted(Action continuation, CancellationToken ct = de { using var _ = _semaphore.Lock(); if (Task == null!) - Task = Start(ct).ContinueWith(continuation, CancellationToken.None); + Task = Start(ct).ContinueWith(continuation, ct); } /// From 4e04c1ea97b67deb43ede2744af1b09694a96adc Mon Sep 17 00:00:00 2001 From: Spike Curtis Date: Thu, 17 Apr 2025 17:21:22 +0400 Subject: [PATCH 3/3] fix comment --- Tests.Vpn.Service/DownloaderTest.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests.Vpn.Service/DownloaderTest.cs b/Tests.Vpn.Service/DownloaderTest.cs index 8bfa367..8b55f50 100644 --- a/Tests.Vpn.Service/DownloaderTest.cs +++ b/Tests.Vpn.Service/DownloaderTest.cs @@ -465,7 +465,7 @@ public async Task ValidationFailureExistingFile(CancellationToken ct) var manager = new Downloader(NullLogger.Instance); var dlTask = await manager.StartDownloadAsync(new HttpRequestMessage(HttpMethod.Get, url), destPath, new TestDownloadValidator(new Exception("test exception")), ct); - // The "outer" Task should fail because the inner task never starts. + // The "inner" Task should fail. var ex = Assert.ThrowsAsync(async () => { await dlTask.Task; }); Assert.That(ex.Message, Does.Contain("Existing file failed validation")); Assert.That(ex.InnerException, Is.Not.Null);