Timeout when getting multiple repositories that have been renamed

[dsplaisted]

The following test fails for me with a TaskCancelledException for the HttpClient.SendAsync call, inside the second call to get a repository:

[IntegrationTest]
public async Task ReturnsRenamedRepository()
{
    var github = Helper.GetAuthenticatedClient();

    var repository = await github.Repository.Get("michael-wolfenden", "Polly");

    Assert.Equal("https://github.com/App-vNext/Polly.git", repository.CloneUrl);
    Assert.False(repository.Private);
    Assert.False(repository.Fork);
    //Assert.Equal(AccountType.User, repository.Owner.Type);

    repository = await github.Repository.Get("fsprojects", "FSharp.Atom");

    Assert.Equal("https://github.com/ionide/ionide-fsharp.git", repository.CloneUrl);
    Assert.False(repository.Private);
    Assert.False(repository.Fork);

    repository = await github.Repository.Get("cabbage89", "Orchard.Weixin");

    Assert.Equal("https://github.com/cabbage89/Orchard.WeChat.git", repository.CloneUrl);
    Assert.False(repository.Private);
    Assert.False(repository.Fork);
}

It does not fail if I instead use repos that have not been renamed:

[IntegrationTest]
public async Task ReturnsRenamedRepository()
{
    var github = Helper.GetAuthenticatedClient();

    var repository = await github.Repository.Get("dotnet", "corefx");

    Assert.Equal("https://github.com/dotnet/corefx.git", repository.CloneUrl);
    Assert.False(repository.Private);
    Assert.False(repository.Fork);
    //Assert.Equal(AccountType.User, repository.Owner.Type);

    repository = await github.Repository.Get("dsplaisted", "PCLStorage");

    Assert.Equal("https://github.com/dsplaisted/PCLStorage.git", repository.CloneUrl);
    Assert.False(repository.Private);
    Assert.False(repository.Fork);

    repository = await github.Repository.Get("paulcbetts", "splat");

    Assert.Equal("https://github.com/paulcbetts/splat.git", repository.CloneUrl);
    Assert.False(repository.Private);
    Assert.False(repository.Fork);
}

It also does not fail if I have Fiddler running.

Full exception spew:

Test Name:  RepositoriesClientTests+TheGetMethod.ReturnsRenamedRepository
Test FullName:  RepositoriesClientTests+TheGetMethod.ReturnsRenamedRepository
Test Source:    C:\git\octokit.net\Octokit.Tests.Integration\Clients\RepositoriesClientTests.cs : line 513
Test Outcome:   Failed
Test Duration:  0:01:41.313

Result StackTrace:  
at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.ConfiguredTaskAwaitable`1.ConfiguredTaskAwaiter.GetResult()
   at Octokit.Internal.HttpClientAdapter.<Send>d__2.MoveNext() in C:\git\octokit.net\Octokit\Http\HttpClientAdapter.cs:line 46
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.ConfiguredTaskAwaitable`1.ConfiguredTaskAwaiter.GetResult()
   at Octokit.Connection.<RunRequest>d__52.MoveNext() in C:\git\octokit.net\Octokit\Http\Connection.cs:line 537
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.ConfiguredTaskAwaitable`1.ConfiguredTaskAwaiter.GetResult()
   at Octokit.Connection.<Run>d__51`1.MoveNext() in C:\git\octokit.net\Octokit\Http\Connection.cs:line 528
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.ConfiguredTaskAwaitable`1.ConfiguredTaskAwaiter.GetResult()
   at Octokit.ApiConnection.<Get>d__9`1.MoveNext() in C:\git\octokit.net\Octokit\Http\ApiConnection.cs:line 83
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter`1.GetResult()
   at RepositoriesClientTests.TheGetMethod.<ReturnsRenamedRepository>d__3.MoveNext() in C:\git\octokit.net\Octokit.Tests.Integration\Clients\RepositoriesClientTests.cs:line 523
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
Result Message: System.Threading.Tasks.TaskCanceledException : A task was canceled.

[daveaglick]

The Fiddler thing has been noted before in #874

[martinscholz83]

This is because if you rename an repo or change the owner you get an 301 moved permanently. In Octokit.Internal.HttpMessageHandlerFactory we set AllowAutoRedirect = false

namespace Octokit.Internal
{
    public static class HttpMessageHandlerFactory
    {
        public static HttpMessageHandler CreateDefault()
        {
            return CreateDefault(null);
        }

        [SuppressMessage("Microsoft.Usage", "CA1801:ReviewUnusedParameters", MessageId = "proxy")]
        [SuppressMessage("Microsoft.Reliability", "CA2000:Dispose objects before losing scope")]
        public static HttpMessageHandler CreateDefault(IWebProxy proxy)
        {
            var handler = new HttpClientHandler
            {
                AllowAutoRedirect = false
            };
#if !PORTABLE
            if (handler.SupportsAutomaticDecompression)
            {
                handler.AutomaticDecompression = DecompressionMethods.GZip | DecompressionMethods.Deflate;
            }
            if (handler.SupportsProxy && proxy != null)
            {
                handler.UseProxy = true;
                handler.Proxy = proxy;
            }
#endif
            return handler;
        }
    }
}

I have tested to set this to true, and voila, i get an response which contains the new owner. @ryangribble @shiftkey can you say which consequence it has to set this to true.

[dsplaisted]

OctoKit is setting AllowAutoRedirect to false on the HttpMessageHandler, and then wrapping it with another handler which handles the redirection explicitly.

It appears that RedirectHandler is causing the problem somehow. If I set AllowAutoRedirect to true and remove the RedirectHandler, the problem goes away.

What's the reason for using RedirectHandler instead of using the built-in redirection handling?

[martinscholz83]

It must have something to do with the new Request.

response = await SendAsync(newRequest, cancellationToken).ConfigureAwait(false);

It only works for the first time. If you call it again, then you end in an error.
But If i cancel all pending requests for _http before you send a new request it works.

 public async Task<IResponse> Send(IRequest request, CancellationToken cancellationToken)
        {
            Ensure.ArgumentNotNull(request, "request");

            var cancellationTokenForRequest = GetCancellationTokenForRequest(request, cancellationToken);

            using (var requestMessage = BuildRequestMessage(request))
            {
                // Make the request
                var responseMessage = await _http.SendAsync(requestMessage, HttpCompletionOption.ResponseContentRead, cancellationTokenForRequest).ConfigureAwait(false);
                _http.CancelPendingRequests();
                return await BuildResponse(responseMessage).ConfigureAwait(false);
            }
        }

I don't know why but it seems that the first redirect request is not completed succesfully. I will try to find out why.

[shiftkey]

What's the reason for using RedirectHandler instead of using the built-in redirection handling?

From MSDN:

The Authorization header is cleared on auto-redirects and HttpWebRequest automatically tries to re-authenticate to the redirected location.

Because of how the GitHub API hides private resources and returns a 404 Not Found, we need to be a bit clever here and set the Authorization header explicitly.

[martinscholz83]

The only fix for me is to call _http.CancelPendingRequests() after you get the response. Because for any reason i don't know there is still a request pending which maybe has the same signatur or anything else. I have no idea. @shiftkey what do you think.

[shiftkey]

For reference, here's the source for HttpClient.CancelPendingRequests() - looks like it's just cleaning up a cancellation token.

[martinscholz83]

I think it has to do with the following line. It seems that there is the same request sending twice. But i don't know why because we are building a new request object 😓

[martinscholz83]

@shiftkey i have create pull request #1411. There i put the logic for redirects outside of the delegating handler. Because for any reason if delegating handler sends the same request twice, and this is not possible. The other solution is to cancel alle pending requests see my comment below.

[shiftkey]

@maddin2016 thanks for digging into this! I've made a note to look at that PR when I get a spare moment...