[do not merge] Task/Cache Jenga 1: Fixing the task system to be more predictive

[shana]

Followed by #608

Includes:

• Download portable git zip files to a known location #585 - Downloading zips to a known location
• Essential changes in GitInstaller should prefer local resources before downloading #590
• Fix start/end handlers and tweak Finally callback
  • It's useful to have the success of the state in the Finally handler that is called in-thread (the ITask-based Finally handlers already get this information), so add that to the callback.
  • In order for the API not to be confusing, make the task affinity parameter mandatory on the Finally overloads that run on a separate thread.
• Fix the HookupHandlers method to use OnStart/Finally so that the callbacks get called on the same thread and not spin up new threads just to set flags. They're already running on non-main threads and doing very simple things so this should be fine.
• Fix Unzip task, it wasn't raising OnStart/OnEnd
• Fix a bunch of bugs in the task system related to tasks that run on failure
• Make it so we don't run tasks inside tasks, and instead just run one chain (so it can fail and we can catch it)
• Make the downloads in pairs (file and md5) in a downloader class that can run them all parallel.
• I kept getting deadlocks and it's something to do with how the concurrent scheduler is set up, so replaced it with a scheduler that just fires a thread per task and does nothing smart about it.

[StanleyGoldman]

What effect does taskIsTopOfChain achieve?

[shana]

Let's say we have chains of tasks like this:

chain1: A->B
chain2: M->N
chain3: Y->Z

A is the top of the chain1. B depends on A,. Some method builds this chain and returns B.
M is the top of chain2. N depends on M. Some method builds this chain and returns N.
Y is the top of chain3. Z depends on Y. We build this chain and have a reference to Y.

Y is a chain that we want to always call when A->B and M->N finish. We want:

A->B->Y->Z
M->N->Y->Z

We call B.Then(Y). The default behaviour of Then is to go up the chain of the task we're appending to grab the top of the chain. In this case, we're passing it Y, which is the top, so it appends Y to B, creating A->B->Y->Z.

We call N.Then(Y). Then goes up the chain of Y to get the top of it, which is now A, and appends A to N. We now have M->N->A->B->Y->Z. Oops.

taskIsTopOfChain tells Then not to go up the chain to get the root, and instead just append the one we're sending. This way, we can call N.Then(Y, taskIsTopOfChain: true) and it will not go up the Y tree and will just append Y to N, creating M->N->Y->Z

[shana]

BTW, I've since found a better way to handle calling different tasks depending on the return of the previous task, by hooking up the OnEnd event and appending (calling Then) there depending on the result. Tasks won't be scheduled until after OnEnd is called, and OnEnd is guaranteed to be called for the current task and it provides the task itself as an argument, so it's a much nicer way of switching tasks.

I might take out the OnFailure task codepath completely since between Finally and OnEnd there are already decent ways of handling this situation.

[StanleyGoldman]

So after going through your comments let me interpret this hunk of code..

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

Lines 174 to 199 in e741551

	var afterGitSetup = new ActionTask(CancellationToken, RestartRepository)
	.ThenInUI(InitializeUI);
	Environment.GitExecutablePath = gitExecutablePath;
	Environment.User.Initialize(GitClient);
	if (Environment.IsWindows)
	{
	var task = GitClient
	.GetConfig("credential.helper", GitConfigSource.Global)
	.Then((b, credentialHelper) => {
	if (string.IsNullOrEmpty(credentialHelper))
	{
	Logger.Warning("No Windows CredentialHelper found: Setting to wincred");
	throw new ArgumentNullException(nameof(credentialHelper));
	}
	});
	// if there's no credential helper, set it before restarting the repository
	task.Then(GitClient.SetConfig("credential.helper", "wincred", GitConfigSource.Global), TaskRunOptions.OnFailure)
	.Then(afterGitSetup, taskIsTopOfChain: true);
	// if there's a credential helper, we're good, restart the repository
	task.Then(afterGitSetup, TaskRunOptions.OnSuccess, taskIsTopOfChain: true);
	}
	afterGitSetup.Start();

1. We setup afterGitSetup with two tasks in it.
   A -> B
2. If we are on windows
   a. We setup two tasks that read a config and throw if it's now found.
   C -> D
   b. We create a task that sets the config. E And schedule it to run on D's failure with afterGitSetup to run after it.
   C -> D -> (fail: E -> A -> B)
   question: you are using taskIsTopofChain here but that has no effect here
   c. We also set afterGitSetup to run on D's success.
   C -> D -> (success: A -> B ; fail: E -> A -> B)
   question: you are using taskIsTopofChain here but that is necessary here
3. Start afterGitSetup

[shana]

Yeah, that's it. I'm using taskIsTopOfChain on both because it's very easy to reorder those instructions and break without realizing it. The intent is to append that task and its children only* onto the chain. Whether there are more tasks above it or not is a detail I don't want to have to worry about later, basically.

[StanleyGoldman]

Do you want to download both if one or the other isn't found?

[shana]

Good point, probably not.

[StanleyGoldman]

By adding to the then we are bypassing the finally that is going to get attached to this task?

[StanleyGoldman]

Unity/src/GitHub.Api/Git/RepositoryManager.cs

Lines 338 to 340 in 8b07cb2

	// we're appending a new continuation after HookupHandlers has run,
	// we need to set the finally handler manually so it runs at the end of everything
	.Finally(s =>

You tweaked the message here, but what happened to the original Finally that HookupHandlers attached to.

[shana]

The Finally has already been appended to the task. This code executes in the body of the task, where HookupHandlers executes before the task runs. The Finally won't be added to the new task that we're appending to the currently executing task, because Finally and Catch handlers only propagate up the chain, not down the chain.

If chain Y+Finally is appended to A->B, the finally handler gets set on all the tasks up to the top so that if any of them fail, it will always get called. This means the chain will be A+Finally->B+Finally->Y+Finally. This is mainly because it's simpler (I only have to deal with one finally handler per task, not many), and 99% of the times we finish building a chain by appending the one that has the finally handler (or we set up the finally handler after we're done building the chain).

In this particular case, though, we're hacking into the chain mid-execution to insert something in between. The chain is A+Finally and we're going A+Finally->B. That means that as soon as A is done, it looks and sees that B is there and calls B. When B is done, it doesn't see any finally handler on B so it doesn't call it. Which is why I'm adding the Finally here, to make sure it gets set at the end.

This isn't really a fix, it's a hack - there are proper ways to fix this. Most likely I would change the way I store finally handlers to always be at the root of the chain, and just go up the chain to call them whenever the chain is finished. That might cause some subtle differences in behaviour that need some more thorough testing.

[StanleyGoldman]

Okay, I get it.

[StanleyGoldman]

If this isGitExtractedTask has already ended. How is this different from just starting extractTask?

[shana]

This is a preparatory change for having a single installer task that you can wait on and get a result from. In that sense, if you Start() a task inside another task, the executing task will then finish and return immediately without waiting for the task you just started to finish. This whole model of sending success/failure tasks that get started from within another task doesn't work very well and I've reworked this entire installer to get rid of that.