Minor improvements regarding UpdateManager and LegalChecker
#12121
Conversation
| @@ -29,8 +29,8 @@ public ClearnetHttpClient(HttpClient httpClient, Func<Uri>? baseUriGetter) | |||
| private HttpClient HttpClient { get; } | |||
|
|
|||
| /// <inheritdoc cref="HttpClient.SendAsync(HttpRequestMessage, CancellationToken)"/> | |||
| public virtual Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken token = default) | |||
| public virtual Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) | |||
There was a problem hiding this comment.
Continuation of the effort to remove defaults in the networking code.
| public event EventHandler<UpdateStatus>? UpdateAvailableToGet; | ||
|
|
||
| public string InstallerDir { get; } | ||
| public IHttpClient HttpClient { get; } | ||
|
|
||
| ///<summary> Comes from config file. Decides Wasabi should download the new installer in the background or not.</summary> | ||
| public bool DownloadNewVersion { get; } | ||
|
|
||
| ///<summary> Install new version on shutdown or not.</summary> | ||
| public bool DoUpdateOnClose { get; set; } | ||
|
|
||
| private UpdateChecker? UpdateChecker { get; set; } | ||
| private CancellationToken CancellationToken { get; set; } |
There was a problem hiding this comment.
This was moved up. I'm not sure why it's here.
| public void Initialize(UpdateChecker updateChecker, CancellationToken cancelationToken) | ||
| { | ||
| UpdateChecker = updateChecker; | ||
| CancellationToken = cancelationToken; | ||
| updateChecker.UpdateStatusChanged += UpdateChecker_UpdateStatusChangedAsync; | ||
| } |
kiminuo
force-pushed
the
feature/2023-12-21-Minor-changes
branch
from
ea1db52
to
df4b31c
Compare
| WasabiClient = Synchronizer.HttpClientFactory.SharedWasabiClient; | ||
| Synchronizer.PropertyChanged += Synchronizer_PropertyChanged; | ||
| } | ||
|
|
||
| public event EventHandler<UpdateStatus>? UpdateStatusChanged; | ||
|
|
||
| private WasabiSynchronizer Synchronizer { get; } | ||
| public UpdateStatus UpdateStatus { get; private set; } | ||
| private UpdateStatus UpdateStatus { get; set; } |
There was a problem hiding this comment.
This does not need to be public.
| @@ -12,15 +12,15 @@ public class UpdateChecker : PeriodicRunner | |||
| public UpdateChecker(TimeSpan period, WasabiSynchronizer synchronizer) : base(period) | |||
| { | |||
| Synchronizer = synchronizer; | |||
| UpdateStatus = new UpdateStatus(true, true, new Version(), 0, new Version()); | |||
| UpdateStatus = new UpdateStatus(backendCompatible: true, clientUpToDate: true, new Version(), currentBackendMajorVersion: 0, new Version()); | |||
There was a problem hiding this comment.
Just to show what is what.
| public string LegalFolder { get; } | ||
| public string ProvisionalLegalFolder { get; } | ||
| public LegalDocuments? CurrentLegalDocument { get; private set; } | ||
| private LegalDocuments? ProvisionalLegalDocument { get; set; } | ||
| private TaskCompletionSource<LegalDocuments> LatestDocumentTaskCompletion { get; } = new(); | ||
|
|
||
| public async Task InitializeAsync(UpdateChecker updateChecker) |
There was a problem hiding this comment.
Passed through ctor.
| @@ -89,8 +95,7 @@ public static async Task VerifyInstallerFileHashesAsync(string[] finalFiles, str | |||
| public static async Task<byte[]> GetShaComputedBytesOfFileAsync(string filePath, CancellationToken cancellationToken = default) | |||
| { | |||
| byte[] bytes = await File.ReadAllBytesAsync(filePath, cancellationToken).ConfigureAwait(false); | |||
| using SHA256 sha = SHA256.Create(); | |||
| byte[] computedHash = sha.ComputeHash(bytes); | |||
| byte[] computedHash = SHA256.HashData(bytes); | |||
There was a problem hiding this comment.
This is new API that does not require additional allocation.
| @@ -30,12 +30,18 @@ public static async Task VerifySha256SumsFileAsync(string sha256SumsAscFilePath) | |||
| // Read the signature file | |||
| var wasabiSignatureFilePath = Path.ChangeExtension(sha256SumsAscFilePath, "wasabisig"); | |||
| string signatureText = await File.ReadAllTextAsync(wasabiSignatureFilePath).ConfigureAwait(false); | |||
| byte[] sigBytes = Convert.FromBase64String(signatureText); | |||
| var wasabiSignature = ECDSASignature.FromDER(sigBytes); | |||
There was a problem hiding this comment.
Extracted to VerifySha256Sum, that method can be more easily tested (not in this PR).
kiminuo
changed the title
UpdateManager and `LegalCheckerUpdateManager and LegalChecker
| private UpdateChecker UpdateChecker { get; } | ||
| private CancellationToken CancellationToken { get; set; } | ||
|
|
||
| public void Initialize(CancellationToken cancelationToken) |
There was a problem hiding this comment.
I tried to get rid of this function and failed miserably. (unless I make some questionable changes)
Do you know why we need THE application stopping cancel token here?
Wouldn't a local CTS be enough?
We could cancel and dispose the local CTS when the Dispose() method is called on stopping WW.
We could create that local CTS in ctor, then we don't need this method anymore.
WDYT?
There was a problem hiding this comment.
I just had a quick look and it seems to me that you are right, a local CTS should be sufficient here. Please open a PR for this, I think it would be a nice cleanup.
btw: What I don't like is
private async void UpdateChecker_UpdateStatusChangedAsync(object? sender, UpdateStatus updateStatus)where we use async code with async void instead of async Task. This can crash the software if there is an unhandled exception IMO.
| UpdateChecker = updateChecker; | ||
| UpdateChecker.UpdateStatusChanged += UpdateChecker_UpdateStatusChangedAsync; |
There was a problem hiding this comment.
Nit:
We could move up the event subscription to the ctor as well, so they are next to each other.
There was a problem hiding this comment.
So I don't think it's a trivial change because between ctor and initialize it would behave differently. It's likely OK but it needs to be explained why it's correct. If you like, you can open a PR for this.
Extracted from #12094
Please review commit by commit.