-
-
Notifications
You must be signed in to change notification settings - Fork 455
Rework how we fetch community plugin data #2222
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
f7b5858
194dbab
64f0da4
f03ac76
2d6b476
df149fa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| using Flow.Launcher.Infrastructure.Http; | ||
| using Flow.Launcher.Infrastructure.Logger; | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Net; | ||
| using System.Net.Http; | ||
| using System.Net.Http.Json; | ||
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
|
|
||
| namespace Flow.Launcher.Core.ExternalPlugins | ||
| { | ||
| public record CommunityPluginSource(string ManifestFileUrl) | ||
| { | ||
| private string latestEtag = ""; | ||
|
|
||
| private List<UserPlugin> plugins = new(); | ||
|
|
||
| /// <summary> | ||
| /// Fetch and deserialize the contents of a plugins.json file found at <see cref="ManifestFileUrl"/>. | ||
| /// We use conditional http requests to keep repeat requests fast. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// This method will only return plugin details when the underlying http request is successful (200 or 304). | ||
| /// In any other case, an exception is raised | ||
| /// </remarks> | ||
| public async Task<List<UserPlugin>> FetchAsync(CancellationToken token) | ||
| { | ||
| Log.Info(nameof(CommunityPluginSource), $"Loading plugins from {ManifestFileUrl}"); | ||
|
|
||
| var request = new HttpRequestMessage(HttpMethod.Get, ManifestFileUrl); | ||
|
|
||
| request.Headers.Add("If-None-Match", latestEtag); | ||
|
|
||
| using var response = await Http.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, token).ConfigureAwait(false); | ||
|
|
||
| if (response.StatusCode == HttpStatusCode.OK) | ||
| { | ||
| this.plugins = await response.Content.ReadFromJsonAsync<List<UserPlugin>>(cancellationToken: token).ConfigureAwait(false); | ||
| this.latestEtag = response.Headers.ETag.Tag; | ||
|
|
||
| Log.Info(nameof(CommunityPluginSource), $"Loaded {this.plugins.Count} plugins from {ManifestFileUrl}"); | ||
| return this.plugins; | ||
| } | ||
| else if (response.StatusCode == HttpStatusCode.NotModified) | ||
| { | ||
| Log.Info(nameof(CommunityPluginSource), $"Resource {ManifestFileUrl} has not been modified."); | ||
| return this.plugins; | ||
| } | ||
| else | ||
| { | ||
| Log.Warn(nameof(CommunityPluginSource), $"Failed to load resource {ManifestFileUrl} with response {response.StatusCode}"); | ||
| throw new Exception($"Failed to load resource {ManifestFileUrl} with response {response.StatusCode}"); | ||
| } | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
|
|
||
| namespace Flow.Launcher.Core.ExternalPlugins | ||
| { | ||
| /// <summary> | ||
| /// Describes a store of community-made plugins. | ||
| /// The provided URLs should point to a json file, whose content | ||
| /// is deserializable as a <see cref="UserPlugin"/> array. | ||
| /// </summary> | ||
| /// <param name="primaryUrl">Primary URL to the manifest json file.</param> | ||
| /// <param name="secondaryUrls">Secondary URLs to access the <paramref name="primaryUrl"/>, for example CDN links</param> | ||
| public record CommunityPluginStore(string primaryUrl, params string[] secondaryUrls) | ||
| { | ||
| private readonly List<CommunityPluginSource> pluginSources = | ||
| secondaryUrls | ||
| .Append(primaryUrl) | ||
| .Select(url => new CommunityPluginSource(url)) | ||
| .ToList(); | ||
|
|
||
| public async Task<List<UserPlugin>> FetchAsync(CancellationToken token, bool onlyFromPrimaryUrl = false) | ||
| { | ||
| // we create a new cancellation token source linked to the given token. | ||
| // Once any of the http requests completes successfully, we call cancel | ||
| // to stop the rest of the running http requests. | ||
| var cts = CancellationTokenSource.CreateLinkedTokenSource(token); | ||
|
|
||
| var tasks = onlyFromPrimaryUrl | ||
| ? new() { pluginSources.Last().FetchAsync(cts.Token) } | ||
| : pluginSources.Select(pluginSource => pluginSource.FetchAsync(cts.Token)).ToList(); | ||
|
|
||
| var pluginResults = new List<UserPlugin>(); | ||
|
|
||
| // keep going until all tasks have completed | ||
| while (tasks.Any()) | ||
| { | ||
| var completedTask = await Task.WhenAny(tasks); | ||
| if (completedTask.IsCompletedSuccessfully) | ||
| { | ||
| // one of the requests completed successfully; keep its results | ||
| // and cancel the remaining http requests. | ||
| pluginResults = await completedTask; | ||
| cts.Cancel(); | ||
| } | ||
| tasks.Remove(completedTask); | ||
| } | ||
|
|
||
| // all tasks have finished | ||
| return pluginResults; | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,26 +49,6 @@ internal PluginsManager(PluginInitContext context, Settings settings) | |
| Settings = settings; | ||
| } | ||
|
|
||
| private Task _downloadManifestTask = Task.CompletedTask; | ||
|
|
||
| internal Task UpdateManifestAsync(CancellationToken token = default, bool silent = false) | ||
| { | ||
| if (_downloadManifestTask.Status == TaskStatus.Running) | ||
| { | ||
| return _downloadManifestTask; | ||
| } | ||
| else | ||
| { | ||
| _downloadManifestTask = PluginsManifest.UpdateManifestAsync(token); | ||
| if (!silent) | ||
| _downloadManifestTask.ContinueWith(_ => | ||
| Context.API.ShowMsg(Context.API.GetTranslation("plugin_pluginsmanager_update_failed_title"), | ||
| Context.API.GetTranslation("plugin_pluginsmanager_update_failed_subtitle"), icoPath, false), | ||
| TaskContinuationOptions.OnlyOnFaulted); | ||
| return _downloadManifestTask; | ||
| } | ||
| } | ||
|
|
||
| internal List<Result> GetDefaultHotKeys() | ||
| { | ||
| return new List<Result>() | ||
|
|
@@ -182,9 +162,9 @@ internal async Task InstallOrUpdateAsync(UserPlugin plugin) | |
| Context.API.RestartApp(); | ||
| } | ||
|
|
||
| internal async ValueTask<List<Result>> RequestUpdateAsync(string search, CancellationToken token) | ||
| internal async ValueTask<List<Result>> RequestUpdateAsync(string search, CancellationToken token, bool usePrimaryUrlOnly = false) | ||
| { | ||
| await UpdateManifestAsync(token); | ||
| await PluginsManifest.UpdateManifestAsync(token, usePrimaryUrlOnly); | ||
|
|
||
| var resultsForUpdate = | ||
| from existingPlugin in Context.API.GetAllPlugins() | ||
|
|
@@ -357,9 +337,9 @@ private bool InstallSourceKnown(string url) | |
| return url.StartsWith(acceptedSource) && Context.API.GetAllPlugins().Any(x => x.Metadata.Website.StartsWith(contructedUrlPart)); | ||
| } | ||
|
|
||
| internal async ValueTask<List<Result>> RequestInstallOrUpdate(string search, CancellationToken token) | ||
| internal async ValueTask<List<Result>> RequestInstallOrUpdate(string search, CancellationToken token, bool usePrimaryUrlOnly = false) | ||
| { | ||
| await UpdateManifestAsync(token); | ||
| await PluginsManifest.UpdateManifestAsync(token, usePrimaryUrlOnly); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we fetch result every time, the ctrl+r will only refresh the result on the first one, but later one will still use the manifest in cdn? I believe we should have local cache for this. (Inserting another character will revert back to the old results).
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't fetch every time, I have added a 10-second timeout so that we don't fetch repeatedly while a user is typing. Hitting
so this is only an issue if the user waits 10 seconds before inserting that extra character.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 10 seconds seems to be pretty short? As we have the ctrl+r features, why don't we add a couple minutes timeout?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
it's only meant to avoid multiple http requests while a user is typing
I don't see a reason to: fetching is very fast with the CDN links, and I've added 3 CDN links now so that fetching should be fast for users anywhere in the world (the fastest cdn link will be used in each user's case). It takes around 80ms for me, a 2-minute timeout for a call this quick seems a bit unreasonable 😂
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, I can increase the timeout to a couple minutes. Does the rest of the PR look good to you?
yeah that's served by cloudflare, don't think they're very solid in China
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The rest looks good to me. I will take a detail look today. Previously jsdelivr has license in China, so they are able to deliver cdn that located in China, which is much much faster and stabler. |
||
|
|
||
| if (Uri.IsWellFormedUriString(search, UriKind.Absolute) | ||
| && search.Split('.').Last() == zip) | ||
|
|
||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why requery means use the primary url?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to add a way to fetch fresh data (not cached data from a CDN) and this seemed a reasonable solution, if you think of
Ctrl+Ras a refresh mechanism.This is mostly a temporary hack for #2048 honestly, I think we can rethink this as part of #2178
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I previously thought
Last()refers to cdn.