Fail on http status codes >=300 for cURL downloads

[DasSkelett]

Motivation

The NetKAN Inflator service fails from time to time at some mods with the error message

Error reading JArray from JsonReader. Current JsonReader item is not an array: StartObject. Path '', line 1, position 1.

for GitHub hosted mods. It appears to happen in batches.
It must be this line that throws:

CKAN/Netkan/Sources/Github/GithubApi.cs

Lines 42 to 46 in 05406ba

	public IEnumerable<GithubRelease> GetAllReleases(GithubRef reference)
	{
	var json = Call($"repos/{reference.Repository}/releases?per_page=100");
	Log.Debug("Parsing JSON...");
	var releases = JArray.Parse(json);

So the answer the bot gets for the API call is not what it expects (a JSON array).

I suspect the answer it gets is a 403 status code telling that the bot is throttled by GitHub (see https://developer.github.com/v3/#rate-limiting).
However, I can't be sure, because errors that might be thrown during/after the download are hidden by the curl fallback mechanism.
.NETs WebClient throws WebExceptions for non-success status codes (I guess >= 400 or 300, couldn't find relevant documentation).
CurlSharp doesn't by default, but behaves as nothing happened and returns the message content.

Changes

I changed the curl download logic to behave more like the native one, that means "failing" if the status code is >= 300.
https://github.com/masroore/CurlSharp/blob/f9da9aefbc2e26e7ad9e1a242a45e9277b1fc82d/CurlSharp/Enums/CurlOption.cs#L224-L229
(However redirects still work!)
Failing means, that the CurlCode returned won't be OK.
We use this as an indication to throw a (Web)Exception.

This should propagate request errors upwards, and NetKAN's GitHubApi class will throw the original webexception instead of an obscure json error.

With some luck, this tells us what is wrong with GitHub API requests.

---

For testing, you can place something like this in CKAN-netkan/program.cs/Main():

try
{
    Console.WriteLine(Net.DownloadText(new Uri("http://httpstat.us/403")));
}
catch (Exception e)
{
	Console.WriteLine(e);
	throw e;
}

[HebaruSan]

How will this affect GUI? Previously DownloadText never would have thrown, and now it can. While it would be nice to get to the bottom of the GitHub errors, we should make sure that we don't accidentally make the GUI user experience worse.

[techman83]

Do we know which mods are affected? Are they the same ones? I don't note any bulk issues, so that wouldn't lead me to think it was rate limiting.

The couple I've checked are being redirected. In theory you can just run netkan over the file and get the same result.

[HebaruSan]

Right now it's these, but it's different ones every time:

In theory you can just run netkan over the file and get the same result.

That's what makes these errors unique, we've been seeing them for a few months and they are never reproducible locally.

[techman83]

Ahh, so happening pre-bot changeover. Weird.

197997970 [1] INFO CKAN.NetKAN.Processors.QueueHandler (null) - Inflating ScottManleyHeadPack
197997970 [1] INFO CKAN.NetKAN.Validators.HasIdentifierValidator (null) - Validating that metadata contains an identifier
197997970 [1] INFO CKAN.NetKAN.Validators.KrefValidator (null) - Validating that metadata contains valid or null $kref
197997970 [1] INFO CKAN.NetKAN.Processors.Inflator (null) - Input successfully passed pre-validation
197997970 [1] INFO CKAN.NetKAN.Transformers.GithubTransformer (null) - Executing GitHub transformation with #/ckan/github/messierr/SMHPContinued
197997970 [1] INFO CKAN.Net (null) - Using auth token for api.github.com
197998175 [1] INFO CKAN.Net (null) - Download failed, trying with curlsharp...
197998175 [1] INFO CKAN.Curl (null) - Using curl-ca bundle: (none)
197998304 [1] INFO CKAN.Net (null) - Using auth token for api.github.com
197998507 [1] INFO CKAN.Net (null) - Download failed, trying with curlsharp...
197998508 [1] INFO CKAN.Curl (null) - Using curl-ca bundle: (none)
197998630 [1] INFO CKAN.NetKAN.Processors.QueueHandler (null) - Inflation failed, sending error: Error reading JArray from JsonReader. Current JsonReader item is not an array: StartObject. Path '', line 1, position 1.

I could set the inflator to run with debugging for a run and see if we can catch it in more detail.

[DasSkelett]

How will this affect GUI? Previously DownloadText never would have thrown, and now it can. While it would be nice to get to the bottom of the GitHub errors, we should make sure that we don't accidentally make the GUI user experience worse.

Valid concern. The way the method was structured with all the try-catch-throws definitely advertised possible exceptions. But if everything building on it accounts for that is a different question. I'm going to take a look.

Right now it's these, but it's different ones every time:

It has been previously, but the last few days (maybe since the new infrastructure) they are always the same (or at least mostly). It's weird.

[DasSkelett]

So what I found:
Net.DownloadText() is called by

1. Netkan's CachingHttpService of course
2. KspBuildMap.TrySetRemoteBuildMap(), enclosed by proper try-catch
3. RepositoryList.DefaultRepositories(), enclosed by proper try-catch
4. AutoUpdate.FetchLatestReleaseInfo(), which is called in
   a) GUI.OnLoad(), encapsulated in proper try-catch
   b) SettingsDialog.CheckForUpdatesButton_Click(), with try-catch
   c) Cmdline.Upgrade.RunCommand() with ckan as option (so self-update), not enclosed by try-catch. Not sure if I should add a try-cacth there, it's not desperately needed.
   d) And a test.
5. Cmdline.Repo.FetchMasterRepositoryList(), which is called in
   a) Repo.AvailableRepositories(), catched
   b) Repo.AddRepository(), catched.
6. GUI.Main.FetchMasterRepositoryList(), which is called in
   NewRepoDialog.NewRepoDialog_Load(), which catches exceptions too.

Besides some occurrences in netkan and the upgrade command, exceptions during DownloadText should be handled nicely.

[HebaruSan]

OK, let's see what we get...

[DasSkelett]

Should the bot already update itself or not?
Because the error is still the same...

[HebaruSan]

I thought it would update itself, yes. @techman83 ?

[techman83]

Only if it's crashing completely. The cake stuff I was futzing around with after work was trying to solve this bit. I've got the permissions sorted out, but I need three build to call Docker Run, exposing the AWS keys as secret parameters.

The doco didn't have any examples and my C# is way too rusty! I'll make a coffee and kick the inflator

[DasSkelett]

It IS an 403 error. The exact response should be found in the log, but I highly suspect throttling reasons.