-
Notifications
You must be signed in to change notification settings - Fork 128
Add a Job manager #706
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
base: v.next
Are you sure you want to change the base?
Add a Job manager #706
Conversation
| /// <summary> | ||
| /// Gets the shared job manager. | ||
| /// </summary> | ||
| public static JobManager Shared { get; } = new JobManager(null); |
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.
Could this lead to a thread-safety issue? It might be a good idea to use Lazy loading for initialization to ensure safe and efficient singleton creation.
| var array = Jobs.Select(j => j.ToJson()).ToArray(); | ||
| lock (StateLock) | ||
| { | ||
| SaveState(string.Join("\n", array)); |
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.
It looks like this implementation assumes there won't be any newline characters in actual SaveState data. If any serialized job contains a newline in a field like an error message or log. Splitting on \n character could break the deserialization.
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.
+1 on this. Newline-delimited JSON seems really fragile.
| } | ||
| try | ||
| { | ||
| // TODO: Check app manifest for background task declaration and provide better instructions how to register the background task |
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.
Should this requirement be documented in the Readme about silent failures if not configured correctly?
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.
Also, I was curious if the success and failure behavior are consistent across the platforms?
| <PackageReference Include="Microsoft.Maui.Controls" Version="$(MauiVersion)" /> | ||
| <PackageReference Include="Microsoft.Maui.Controls.Compatibility" Version="$(MauiVersion)" /> | ||
| <PackageReference Include="Esri.Calcite.Maui" Version="1.0.0-rc.1" /> | ||
| </ItemGroup> |
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.
This closing tag seems duplicated.
| /// <summary> | ||
| /// The jobs being managed by the job manager. | ||
| /// </summary> | ||
| public IList<IJob> Jobs { get; } = new System.Collections.ObjectModel.ObservableCollection<IJob>(); |
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.
This will reinitialize the Jobs collection every time we call get on Jobs and seems like a bug. We can make use of private backing field which stays for the lifetime of the JobManager.
| public async Task PerformStatusChecks() | ||
| { | ||
| var tasks = Jobs.Select(async job => | ||
| { | ||
| try { await job.CheckStatusAsync(); } catch { } | ||
| }); | ||
| await Task.WhenAll(tasks); | ||
| } |
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.
This process could be made faster using SimaphoreSlim or Parallel.ForEachAsync which supports concurrency.
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.
Probably not -- CheckStatusAsync yields almost immediately, after signaling a background thread in core. So we queue up all the checks and await them in parallel using Task.WhenAll. This is about as efficient as it can get.
| } | ||
| } | ||
|
|
||
| var path = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData), new Guid().ToString() + ".geodatabase"); |
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.
new Guid() is the default constructor -- it's always going to be 00000000-0000-0000-0000-000000000000. Did you mean to do Guid.NewGuid()?
| var path = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData), new Guid().ToString() + ".geodatabase"); | |
| var path = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData), Guid.NewGuid().ToString() + ".geodatabase"); |
| private void OnJobPropertyChanged(IJob? oldJob, IJob? newJob) | ||
| { | ||
| if (oldJob is not null) | ||
| oldJob.ProgressChanged -= Job_ProgressChanged; |
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.
Should this unsubscribe from oldJob.StatusChanged too -- like we do for ProgressChanged?
| /// </summary> | ||
| public void SaveState() | ||
| { | ||
| var array = Jobs.Select(j => j.ToJson()).ToArray(); |
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.
Events like ProgressChanged/StatusChanged will be coming from background threads and calling SaveState. Do we need some locks around this to avoid overlapping saves? Or e.g. what if user adds a Job (modifying Jobs collection) on the UI thread while SaveState is enumerating in the background? That's a recipe for "collection was modified" exceptions.
| if (value != BackgroundStatusCheckSchedule.Disabled && ValidateInfoPlist()) | ||
| _preferredBackgroundStatusCheckSchedule = value; |
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.
If you ever enable this property, it would be impossible to disable due to this if check. Is that intentional?
| return false; | ||
| throw new InvalidOperationException($"'BGTaskSchedulerPermittedIdentifiers' must contain '{DefaultsKey}'."); |
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.
Were you undecided about returning or throwing? The xmldoc on PreferredBackgroundStatusCheckSchedule says that we throw, but the setter actually just fails. The throw is unreachable.
| _isBackgroundStatusChecksScheduled = true; | ||
| var request = new BGAppRefreshTaskRequest(StatusChecksTaskIdentifier) | ||
| { | ||
| EarliestBeginDate = NSDate.Now.AddSeconds(PreferredBackgroundStatusCheckSchedule.Interval) | ||
| }; | ||
| NSError error; | ||
| BGTaskScheduler.Shared.Submit(request, out error); |
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.
Should we perhaps check the error before setting _isBackgroundStatusChecksScheduled to true? It might have failed, e.g.
There can be a total of 1 refresh task and 10 processing tasks scheduled at any time. Trying to schedule more tasks returns BGTaskScheduler.Error.Code.tooManyPendingTaskRequests.
| public class BackgroundStatusCheckSchedule | ||
| { | ||
| public static readonly BackgroundStatusCheckSchedule Disabled = new BackgroundStatusCheckSchedule(); | ||
| public static BackgroundStatusCheckSchedule RegularInterval(double interval) => new BackgroundStatusCheckSchedule { Interval = interval }; | ||
| public double Interval { get; private set; } | ||
| } |
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.
This class could use a bit of doc. Also, it seems that you meant to make this factory-constructible -- should we make the default constructor private?
And while you're at it, the units of interval are not self-explanatory. How about using TimeSpan instead?
| private IBackgroundTaskInstance? _taskInstance; | ||
|
|
||
| [MTAThread] | ||
| public void Run(IBackgroundTaskInstance taskInstance) |
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.
Is the _taskInstance field meant to be initialized here? Nothing assigns to that field right now.
| { | ||
|
|
||
| var taskId = taskInstance.Task.Name; | ||
| manager = JobManager.Create(taskId); |
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.
How does the BackgroundTask's JobManager communicate with the original JobManager that started it? Are they meant to pass state around via the shared state file? That seems pretty fragile -- both managers will be trying to read/write that file with no coordination.
As far as I can tell we never name our BackgroundTaskBuilder, so wouldn't taskId always be empty here?
| var array = Jobs.Select(j => j.ToJson()).ToArray(); | ||
| lock (StateLock) | ||
| { | ||
| SaveState(string.Join("\n", array)); |
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.
+1 on this. Newline-delimited JSON seems really fragile.
A port of the Swift toolkit JobManager. See: https://github.com/Esri/arcgis-maps-sdk-swift-toolkit/blob/117a0a35662f7a2403b22d7048e4f0c53a97c07c/Sources/ArcGISToolkit/Components/JobManager/JobManager.swift