Skip to content
This repository was archived by the owner on Dec 5, 2024. It is now read-only.

Commit c439795

Browse files
shanashana
committed
Refactor how we run tasks synchronously so that TPL tasks aren't broken
Our task system uses TPL tasks (System.Threading.Tasks.Task) under the hood (every ITask really is a TPL task whose body is the callback or Run/RunWithReturn/RunWithData method override), so when we run one of our ITask, what we're really doing is scheduling the underlying TPL task to be executed. If we want to run the ITask synchronously, it's straightforward to just execute the Run/RunWithReturn/RunWithData method that does the actual work. Because of this, we also have an overload to pass in an existing TPL task into an ITask, so that we can also run async/await task types in our threading system (so that we can control the thread these tasks are going to be run in, given that async/await has no threading control model). The problem with allowing a TPL task to be set directly into an ITask is that the TPL task wasn't set up in a way that exposed a Run/RunWithReturn/RunWithData method that could be called synchronously.This PR moves things around so that instead of calling directly the Run/RunWithReturn/RunWithData methods, there is one RunSynchronously method that does the right thing for all task types, regardless of how they're initialized. This has the added benefit that the exact same method (`RunSynchronously`) is executed independent of who's calling it - if the ITask is running on the scheduler, `RunSynchronously` is the task body that the scheduler will execute - if the user wants to run it in thread, they can call it directly.
1 parent ea5312b commit c439795

File tree

17 files changed

+566
-500
lines changed

17 files changed

+566
-500
lines changed

‎src/GitHub.Api/Application/ApplicationManagerBase.cs‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ abstract class ApplicationManagerBase : IApplicationManager
1616
private Progress progress = new Progress(TaskBase.Default);
1717
protected bool isBusy;
1818
private bool firstRun;
19-
protected bool FirstRun { get { return firstRun; } set { firstRun = value; } }
19+
protected bool FirstRun { get { return firstRun; } set { firstRun = value; } }
2020
private Guid instanceId;
2121
protected Guid InstanceId { get { return instanceId; } set { instanceId = value; } }
2222

@@ -75,7 +75,7 @@ public void Run()
7575
var getEnvPath = new SimpleProcessTask(TaskManager.Token, "bash".ToNPath(), "-c \"/usr/libexec/path_helper\"")
7676
.Configure(ProcessManager, dontSetupGit: true)
7777
.Catch(e => true); // make sure this doesn't throw if the task fails
78-
var path = getEnvPath.RunWithReturn(true);
78+
var path = getEnvPath.RunSynchronously();
7979
if (getEnvPath.Successful)
8080
{
8181
Logger.Trace("Existing Environment Path Original:{0} Updated:{1}", Environment.Path, path);
@@ -194,7 +194,7 @@ public void SetupGit(GitInstaller.GitInstallationState state)
194194
Logger.Error(e, "Error running lfs install");
195195
return true;
196196
})
197-
.RunWithReturn(true);
197+
.RunSynchronously();
198198
}
199199

200200
if (Environment.IsWindows)
@@ -204,7 +204,7 @@ public void SetupGit(GitInstaller.GitInstallationState state)
204204
{
205205
Logger.Error(e, "Error getting the credential helper");
206206
return true;
207-
}).RunWithReturn(true);
207+
}).RunSynchronously();
208208

209209
if (string.IsNullOrEmpty(credentialHelper))
210210
{
@@ -215,7 +215,7 @@ public void SetupGit(GitInstaller.GitInstallationState state)
215215
Logger.Error(e, "Error setting the credential helper");
216216
return true;
217217
})
218-
.RunWithReturn(true);
218+
.RunSynchronously();
219219
}
220220
}
221221
}
@@ -238,21 +238,21 @@ public void InitializeRepository()
238238

239239
var filesForInitialCommit = new List<string> { gitignore, gitAttrs, assetsGitignore };
240240

241-
GitClient.Init().RunWithReturn(true);
241+
GitClient.Init().RunSynchronously();
242242
progress.UpdateProgress(10, 100, "Initializing...");
243243

244244
ConfigureMergeSettings();
245245
progress.UpdateProgress(20, 100, "Initializing...");
246246

247-
GitClient.LfsInstall().RunWithReturn(true);
247+
GitClient.LfsInstall().RunSynchronously();
248248
progress.UpdateProgress(30, 100, "Initializing...");
249249

250250
AssemblyResources.ToFile(ResourceType.Generic, ".gitignore", targetPath, Environment);
251251
AssemblyResources.ToFile(ResourceType.Generic, ".gitattributes", targetPath, Environment);
252252
assetsGitignore.CreateFile();
253-
GitClient.Add(filesForInitialCommit).RunWithReturn(true);
253+
GitClient.Add(filesForInitialCommit).RunSynchronously();
254254
progress.UpdateProgress(60, 100, "Initializing...");
255-
GitClient.Commit("Initial commit", null).RunWithReturn(true);
255+
GitClient.Commit("Initial commit", null).RunSynchronously();
256256
progress.UpdateProgress(70, 100, "Initializing...");
257257
Environment.InitializeRepository();
258258
UsageTracker.IncrementProjectsInitialized();
@@ -287,12 +287,12 @@ private void ConfigureMergeSettings()
287287
GitClient.SetConfig("merge.unityyamlmerge.cmd", yamlMergeCommand, GitConfigSource.Local).Catch(e => {
288288
Logger.Error(e, "Error setting merge.unityyamlmerge.cmd");
289289
return true;
290-
}).RunWithReturn(true);
290+
}).RunSynchronously();
291291

292292
GitClient.SetConfig("merge.unityyamlmerge.trustExitCode", "false", GitConfigSource.Local).Catch(e => {
293293
Logger.Error(e, "Error setting merge.unityyamlmerge.trustExitCode");
294294
return true;
295-
}).RunWithReturn(true);
295+
}).RunSynchronously();
296296
}
297297

298298
public void RestartRepository()

‎src/GitHub.Api/Authentication/LoginManager.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ public ITask Logout(UriString hostAddress)
138138
{
139139
Guard.ArgumentNotNull(hostAddress, nameof(hostAddress));
140140

141-
return new ActionTask(keychain.Clear(hostAddress, true)) { Message = "Signing out" }.Start();
141+
return new TPLTask(keychain.Clear(hostAddress, true)) { Message = "Signing out" }.Start();
142142
}
143143

144144
private async Task<LoginResultData> TryLogin(

‎src/GitHub.Api/Installer/GitInstaller.cs‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ private GitInstallationState FindGit(GitInstallationState state)
106106
var gitPath = new FindExecTask("git", cancellationToken)
107107
.Configure(processManager, dontSetupGit: true)
108108
.Catch(e => true)
109-
.RunWithReturn(true);
109+
.RunSynchronously();
110110
state.GitExecutablePath = gitPath;
111111
state = ValidateGitVersion(state);
112112
if (state.GitIsValid)
@@ -122,7 +122,7 @@ private GitInstallationState FindGitLfs(GitInstallationState state)
122122
var gitLfsPath = new FindExecTask("git-lfs", cancellationToken)
123123
.Configure(processManager, dontSetupGit: true)
124124
.Catch(e => true)
125-
.RunWithReturn(true);
125+
.RunSynchronously();
126126
state.GitLfsExecutablePath = gitLfsPath;
127127
state = ValidateGitLfsVersion(state);
128128
if (state.GitLfsIsValid)
@@ -159,7 +159,7 @@ public GitInstallationState ValidateGitVersion(GitInstallationState state)
159159
var version = new GitVersionTask(cancellationToken)
160160
.Configure(processManager, state.GitExecutablePath, dontSetupGit: true)
161161
.Catch(e => true)
162-
.RunWithReturn(true);
162+
.RunSynchronously();
163163
state.GitIsValid = version >= Constants.MinimumGitVersion;
164164
state.GitVersion = version;
165165
return state;
@@ -175,7 +175,7 @@ public GitInstallationState ValidateGitLfsVersion(GitInstallationState state)
175175
var version = new ProcessTask<TheVersion>(cancellationToken, "version", new LfsVersionOutputProcessor())
176176
.Configure(processManager, state.GitLfsExecutablePath, dontSetupGit: true)
177177
.Catch(e => true)
178-
.RunWithReturn(true);
178+
.RunSynchronously();
179179
state.GitLfsIsValid = version >= Constants.MinimumGitLfsVersion;
180180
state.GitLfsVersion = version;
181181
return state;
@@ -244,7 +244,7 @@ private GitInstallationState GetZipsIfNeeded(GitInstallationState state)
244244
if (state.GitZipExists && state.GitLfsZipExists)
245245
return state;
246246

247-
var downloader = new Downloader();
247+
var downloader = new Downloader(environment.FileSystem);
248248
downloader.Catch(e =>
249249
{
250250
LogHelper.Trace(e, "Failed to download");
@@ -255,7 +255,7 @@ private GitInstallationState GetZipsIfNeeded(GitInstallationState state)
255255
downloader.QueueDownload(state.GitPackage.Uri, installDetails.ZipPath);
256256
if (!state.GitLfsZipExists && !state.GitLfsIsValid && state.GitLfsPackage != null)
257257
downloader.QueueDownload(state.GitLfsPackage.Uri, installDetails.ZipPath);
258-
downloader.RunWithReturn(true);
258+
downloader.RunSynchronously();
259259

260260
state.GitZipExists = installDetails.GitZipPath.FileExists();
261261
state.GitLfsZipExists = installDetails.GitLfsZipPath.FileExists();
@@ -295,7 +295,7 @@ private GitInstallationState ExtractGit(GitInstallationState state)
295295
return true;
296296
});
297297
unzipTask.Progress(p => Progress.UpdateProgress(40 + (long)(20 * p.Percentage), 100, unzipTask.Message));
298-
var path = unzipTask.RunWithReturn(true);
298+
var path = unzipTask.RunSynchronously();
299299
var target = state.GitInstallationPath;
300300
if (unzipTask.Successful)
301301
{
@@ -320,7 +320,7 @@ private GitInstallationState ExtractGit(GitInstallationState state)
320320
return true;
321321
});
322322
unzipTask.Progress(p => Progress.UpdateProgress(60 + (long)(20 * p.Percentage), 100, unzipTask.Message));
323-
var path = unzipTask.RunWithReturn(true);
323+
var path = unzipTask.RunSynchronously();
324324
var target = state.GitLfsInstallationPath;
325325
if (unzipTask.Successful)
326326
{

‎src/GitHub.Api/Installer/OctorunInstaller.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ public NPath SetupOctorunIfNeeded()
3838
tempZipExtractPath, sharpZipLibHelper,
3939
fileSystem)
4040
.Catch(e => { Logger.Error(e, "Error extracting octorun"); return true; });
41-
var extractPath = unzipTask.RunWithReturn(true);
41+
var extractPath = unzipTask.RunSynchronously();
4242
if (unzipTask.Successful)
4343
path = MoveOctorun(extractPath.Combine("octorun"));
4444
return path;
@@ -112,4 +112,4 @@ public OctorunInstallDetails(NPath baseDataPath)
112112
public NPath VersionFile => InstallationPath.Combine("version");
113113
}
114114
}
115-
}
115+
}

‎src/GitHub.Api/Installer/UnzipTask.cs‎

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -26,24 +26,17 @@ protected NPath BaseRun(bool success)
2626
return base.RunWithReturn(success);
2727
}
2828

29-
public override NPath RunWithReturn(bool success)
29+
protected override NPath RunWithReturn(bool success)
3030
{
3131
var ret = BaseRun(success);
32-
33-
RaiseOnStart();
34-
3532
try
3633
{
3734
ret = RunUnzip(success);
3835
}
3936
catch (Exception ex)
4037
{
4138
if (!RaiseFaultHandlers(ex))
42-
throw;
43-
}
44-
finally
45-
{
46-
RaiseOnEnd(ret);
39+
throw exception;
4740
}
4841
return ret;
4942
}

‎src/GitHub.Api/Managers/Downloader.cs‎

Lines changed: 24 additions & 100 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,7 @@
11
using System;
2-
using System.Collections.Generic;
32
using System.IO;
43
using System.Net;
5-
using System.Threading;
6-
using System.Threading.Tasks;
74
using GitHub.Logging;
8-
using System.Linq;
95

106
namespace GitHub.Unity
117
{
@@ -20,111 +16,39 @@ public DownloadData(UriString url, NPath file)
2016
}
2117
}
2218

23-
class Downloader : FuncListTask<DownloadData>
19+
class Downloader : TaskQueue<NPath, DownloadData>
2420
{
25-
public event Action<DownloadData> DownloadStart;
26-
public event Action<DownloadData> DownloadComplete;
27-
public event Action<DownloadData, Exception> DownloadFailed;
21+
public event Action<UriString> OnDownloadStart;
22+
public event Action<UriString, NPath> OnDownloadComplete;
23+
public event Action<UriString, Exception> OnDownloadFailed;
2824

29-
private readonly List<DownloaderTask> downloaders = new List<DownloaderTask>();
30-
31-
public override string Message { get; set; } = "Downloading...";
32-
33-
public Downloader() : base(TaskManager.Instance.Token, RunDownloaders)
25+
private readonly IFileSystem fileSystem;
26+
public Downloader(IFileSystem fileSystem)
27+
: base(t =>
28+
{
29+
var dt = t as DownloadTask;
30+
var destinationFile = dt.TargetDirectory.Combine(dt.Url.Filename);
31+
return new DownloadData(dt.Url, destinationFile);
32+
})
3433
{
34+
this.fileSystem = fileSystem;
3535
Name = "Downloader";
36+
Message = "Downloading...";
3637
}
3738

3839
public void QueueDownload(UriString url, NPath targetDirectory)
3940
{
40-
var downloaderTask = new DownloaderTask();
41-
downloaderTask.QueueDownload(url, targetDirectory);
42-
downloaders.Add(downloaderTask);
43-
}
44-
45-
private static List<DownloadData> RunDownloaders(bool success, FuncListTask<DownloadData> source)
46-
{
47-
Downloader self = (Downloader)source;
48-
List<DownloadData> result = null;
49-
var listOfTasks = new List<Task<DownloadData>>();
50-
foreach (var downloader in self.downloaders)
51-
{
52-
downloader.DownloadStart += self.DownloadStart;
53-
downloader.DownloadComplete += self.DownloadComplete;
54-
downloader.DownloadFailed += self.DownloadFailed;
55-
listOfTasks.Add(downloader.Run());
56-
}
57-
var res = TaskEx.WhenAll(listOfTasks).Result;
58-
if (res != null)
59-
result = new List<DownloadData>(res);
60-
return result;
61-
}
62-
63-
class DownloaderTask
64-
{
65-
public event Action<DownloadData> DownloadStart;
66-
public event Action<DownloadData> DownloadComplete;
67-
public event Action<DownloadData, Exception> DownloadFailed;
68-
69-
private readonly List<ITask<NPath>> queuedTasks = new List<ITask<NPath>>();
70-
private readonly TaskCompletionSource<DownloadData> aggregateDownloads = new TaskCompletionSource<DownloadData>();
71-
private readonly IFileSystem fs;
72-
private readonly CancellationToken cancellationToken;
73-
74-
private volatile bool isSuccessful = true;
75-
private volatile Exception exception;
76-
private DownloadData result;
77-
78-
public DownloaderTask()
79-
{
80-
fs = NPath.FileSystem;
81-
cancellationToken = TaskManager.Instance.Token;
82-
DownloadComplete += d => aggregateDownloads.TrySetResult(d);
83-
DownloadFailed += (_, e) => aggregateDownloads.TrySetException(e);
84-
}
85-
86-
public Task<DownloadData> Run()
87-
{
88-
foreach (var task in queuedTasks)
89-
task.Start();
90-
if (queuedTasks.Count == 0)
91-
DownloadComplete(result);
92-
return aggregateDownloads.Task;
93-
}
94-
95-
public Task<DownloadData> QueueDownload(UriString url, NPath targetDirectory)
41+
var download = new DownloadTask(Token, fileSystem, url, targetDirectory);
42+
download.OnStart += t => OnDownloadStart?.Invoke(((DownloadTask)t).Url);
43+
download.OnEnd += (t, res, s, ex) =>
9644
{
97-
var destinationFile = targetDirectory.Combine(url.Filename);
98-
result = new DownloadData(url, destinationFile);
99-
100-
Action<ITask<NPath>, NPath, bool, Exception> verifyDownload = (t, res, success, ex) =>
101-
{
102-
isSuccessful &= success;
103-
if (!success)
104-
exception = ex;
105-
if (!isSuccessful)
106-
{
107-
DownloadFailed(result, exception);
108-
}
109-
else
110-
{
111-
DownloadComplete(result);
112-
}
113-
};
114-
115-
var fileDownload = DownloadFile(url, targetDirectory, result, verifyDownload);
116-
fileDownload.OnStart += _ => DownloadStart?.Invoke(result);
117-
queuedTasks.Add(fileDownload);
118-
return aggregateDownloads.Task;
119-
}
120-
121-
private ITask<NPath> DownloadFile(UriString url, NPath targetDirectory, DownloadData res, Action<ITask<NPath>, NPath, bool, Exception> verifyDownload)
122-
{
123-
var download = new DownloadTask(cancellationToken, fs, url, targetDirectory)
124-
.Catch(e => { DownloadFailed(res, e); return true; });
125-
download.OnEnd += verifyDownload;
126-
return download;
127-
}
45+
if (s)
46+
OnDownloadComplete?.Invoke(((DownloadTask)t).Url, res);
47+
else
48+
OnDownloadFailed?.Invoke(((DownloadTask)t).Url, ex);
49+
};
50+
// queue after hooking up events so OnDownload* gets called first
51+
Queue(download);
12852
}
12953

13054
public static bool Download(ILogging logger, UriString url,

‎src/GitHub.Api/Primitives/Package.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ public static Package Load(IEnvironment environment, UriString packageFeed)
4141
LogHelper.Warning(@"Error downloading package feed:{0} ""{1}"" Message:""{2}""", packageFeed, ex.GetType().ToString(), ex.GetExceptionMessageShort());
4242
return true;
4343
})
44-
.RunWithReturn(true);
44+
.RunSynchronously();
4545

4646
if (feed.IsInitialized)
4747
environment.UserSettings.Set<DateTimeOffset>(key, now);
@@ -67,4 +67,4 @@ public static Package Load(IEnvironment environment, UriString packageFeed)
6767
return package;
6868
}
6969
}
70-
}
70+
}

0 commit comments

Comments
 (0)