Fix redirect timeout on renamed repositories #1411

martinscholz83 · 2016-07-05T08:34:45Z

Put logic for redirects outside delegatinghandler in HttpClientAdapter.

…imeout-getting-multiple-repositories

ryangribble · 2016-07-05T11:22:45Z

Octokit/Http/HttpClientAdapter.cs

+                }
+                newRequest.RequestUri = response.Headers.Location;
+
+                if (string.Compare(newRequest.RequestUri.Host, newRequest.RequestUri.Host, StringComparison.OrdinalIgnoreCase) != 0)


this is comparing the same variable. the old code was newRequest vs request

ryangribble · 2016-07-05T11:50:35Z

I have just been troubleshooting a problem with the GitHub Enterprise management console stuff I'm implementing at the moment, where I was getting hanging tasks in my integration tests... it turns out it's the same problem you are fixing here! 😀 (management console uses redirects which as we've seen seems to trigger this behaviour).

The good news is, with your fix in place (modified a bit) I no longer get the deadlock 😀

Your code changes were pretty hard to diff due to moving the functions around in the file. I actually re-organised it so the GetSuccessfulResponse and CopyRequest functions were last in the class, which then made the diff line up much nicer and revealed only a few lines of difference. This let me spot a few places where you are not doing exactly what the old code did which I've highlighted in the review comments.

I fixed these locally, plus fixed up a few async/await type issues and renamed GetSuccessfulResponse to SendAsync and everything is working 💎 for me now with the problem I was facing. I added what I ended up with to this gist if you wanted to see

There is still the matter of other failed unit tests around redirects and so on to tackle though...

martinscholz83 · 2016-07-05T12:13:30Z

I rewrite the failed unit tests

shiftkey · 2016-07-06T00:34:25Z

Just throwing myself on as a reminder to sit down and properly understand this change 🤘

shiftkey · 2016-07-11T23:02:16Z

@maddin2016 have you had a chance to look at those failing tests?

martinscholz83 · 2016-07-12T09:05:07Z

Not really. Maybe in the next few days. What i have seen is that we have to completely rewright tests for redirects. Yet they are in RedirectHandlerTests but i think we should move them to HttpClientAdapterTests because redirecting is now handled directly in there. What do you think?

shiftkey · 2016-07-12T18:19:36Z

@maddin2016 I'll look at those failing tests and see if I can help

martinscholz83 · 2016-07-14T21:33:30Z

Hi @shiftkey. I have sit down and think about these failing redirect tests. Here is an approach in which i have move one test into HttpClientAdapterTests. Because redirecting is handled by this class. But to translate tests i have to create a new property RequestMessage for class Response to get access to the RequestMessage. What do you think? So this is really a first approach!!

martinscholz83 · 2016-07-15T08:40:33Z

I have try to copy these other tests and have find that is not possible to copy them 1 to 1. Because i always have to create new properties for IResponse and IRequest like Authorization. What is the intent for these both classes instead of using HttpRequestMessage and HttpResponse. Because you anyway translate them into HttpRequestMessage like here

ryangribble · 2016-07-16T10:13:55Z

Yeah I dont think we want to add these properties to IResponse just so we can "test" what request was used.

I had a go at re-jigging the tests with the new redirect handling like you've got in this PR. I found that inside the redirect method, I had to pre-save off the original request's content so it could be later added to the request for the redirected Url, as by that stage a Object has been disposed exception was thrown when trying to access the original request's content directly. With this change, and some modifications to the test/mocks I was able to get all the tests passing, except for the Assert that tries to verify that same field (response.RequestMessage.Content). If you wanted to take a look at what I did, the commit is here: TattsGroup@c6f6654

…imeout-getting-multiple-repositories

shiftkey · 2016-08-01T01:36:31Z

Octokit/Http/HttpClientAdapter.cs

+
+    internal class RedirectHandler : DelegatingHandler
+    {
+        public bool Enabled { get; set; }


I don't see this value being used anywhere (I suspect it also wasn't used before).

shiftkey · 2016-08-01T01:39:57Z

One thing I would like to comment is that we now have two public send methods in HttpClientAdapter. Both are asynchronous but one is called Send and the other SendAsync. Is this ok?

This is fine. I'd rather not break this API now, and come back to it when we want to let people provide their own HttpClient implementation.

shiftkey · 2016-08-01T01:42:20Z

Octokit/Http/HttpClientAdapter.cs

+                else
+                {
+                    unbuffered = true;
+                    //throw new Exception("Cannot redirect a request with an unbuffered body");


💄 let's 🔥 these commented lines

shiftkey · 2016-08-01T01:45:37Z

@maddin2016 i like where this is heading, and I think this is close to merging in.

I'd like to push this out as a release on it's own so we can verify it in isolation from other features being rolled out 🤘

ryangribble · 2016-08-01T02:42:37Z

From memory the main issue i found with these changes was that you can no longer "reach in" and access the original request message (response.Request. Content) as it's been disposed.

I'm not sure if anything really needs to do this (it was only the integration test that was doing it in our code)

shiftkey · 2016-08-01T09:34:03Z

@ryangribble I'm okay with locking that down if it means resolving this redirect issue - it was supposed to be a black box and I'm not really hearing anyone reimplement our IHttpClient without significant pain - and they'll likely be not using this stuff anyway.

If someone comes up with a good use case for needing to poke the request content after we lock this down, then I'm all ears...

martinscholz83 · 2016-08-01T11:24:57Z

@shiftkey this means these changes from @ryangribble are ok?

shiftkey · 2016-08-09T01:50:57Z

@maddin2016 yep, those comments are fine. Just a couple of tweaks I think to get this ready to merge.

martinscholz83 · 2016-08-09T17:20:04Z

Something I can do to get ready?

alexander-efremov · 2016-08-09T17:40:03Z

@maddin2016 I guess @shiftkey talking about his comments in your last commits. If you delete commented/dead code and do something else it can be merged.

https://github.com/octokit/octokit.net/pull/1411/files#diff-6792693a30870bb1388f1ae706bbedd7R265

martinscholz83 · 2016-08-09T20:43:16Z

Thanks for that hint @dampir. I thought there where unresolved things because of

I'm not sure if anything really needs to do this (it was only the integration test that was doing it in our code)

I will asap fix the comments from @shiftkey.

ryangribble · 2016-08-10T13:55:23Z

Hey so I pulled these changes down and was going through things one final time and I decided to unskip the failing test from #874 now that we "should" be handling redirects properly. And unfortunately it failed 😡 The problem was something related to the way I had saved off the content stream from the original request, and the new content still had a reference to that old stream.

So anyway, I did some more digging and have come up with a revised implementation that doesn't have issues with content streams and looks tidier too. With this fix all the existing tests are passing and unskipping the one from #874 now works too 👍

The commit is TattsGroup@168ea41, if you wanted to cherrypick it @maddin2016 and take a look?

…se it later Now the skipped test from octokit#874 works! Also had to fix the new ReturnsRenamedRepository test as the ionide repo was renamed again

martinscholz83 · 2016-08-11T06:34:58Z

Hi @ryangribble, do you mean test CanCreateIssueOnRedirectedRepository? Maybe it is better to clone the request after we get the respone? Only for design

            // Send initial response
            var response = await _http.SendAsync(request, HttpCompletionOption.ResponseContentRead, cancellationToken).ConfigureAwait(false);

            // Can't redirect without somewhere to redirect to.
            if (response.Headers.Location == null)
            {
                return response;
            }

// Clone the request/content incase we get a redirect
var clonedRequest = await CloneHttpRequestMessageAsync(request);

ryangribble · 2016-08-11T08:05:35Z

That's the whole problem, you can't get the content stream ad it's been disposed

martinscholz83 · 2016-08-11T08:59:59Z

Ok, that makes sense

ryangribble · 2016-08-12T21:35:49Z

@maddin2016 were you happy with that change? Can you confirm all the related integration tests are working for you, including the one I unskipped?

martinscholz83 · 2016-08-13T08:48:44Z

All tests including that one you have uncomment work for me. So i think we're done, right?

ryangribble · 2016-08-14T20:57:45Z

Awesome stuff, I'll merge this in then

ryangribble · 2016-08-14T20:58:12Z

Thanks @maddin2016

shiftkey · 2016-08-15T04:42:09Z

Thanks for wrapping this up while I was off sick last week.

martinscholz83 · 2016-08-15T06:32:07Z

This fixes #534 via octokit/octokit.net#1411. Previously the request to /site/sha to detect an enterprise instance wasn't getting sent due to octokit's implementation of `RedirectHandler`.

martinscholz83 added 9 commits June 28, 2016 19:40

Merge remote-tracking branch 'refs/remotes/octokit/master'

dcf1cf0

add test

bce9696

Merge remote-tracking branch 'refs/remotes/octokit/master'

61dfff8

Merge remote-tracking branch 'refs/remotes/octokit/master' into fix-t…

dd38e4c

…imeout-getting-multiple-repositories

Merge remote-tracking branch 'refs/remotes/octokit/master'

dc65dab

Merge branch 'master' of https://github.com/maddin2016/octokit.net

f6191d2

Merge remote-tracking branch 'refs/remotes/octokit/master' into fix-t…

7f8cfba

…imeout-getting-multiple-repositories

[WIP]

84f0e7b

put logic for redirects outside of delegating handler

b4d0ea9

martinscholz83 mentioned this pull request Jul 5, 2016

Timeout when getting multiple repositories that have been renamed #1396

Closed

martinscholz83 added 2 commits July 5, 2016 10:43

change send method

d2748c3

format code

f3efff1

martinscholz83 force-pushed the fix-timeout-getting-multiple-repositories branch from 30f058e to f3efff1 Compare July 5, 2016 08:57

ryangribble reviewed Jul 5, 2016
View reviewed changes

reorganized http client adapter

b42240b

ryangribble mentioned this pull request Jul 5, 2016

Enterprise ManagementConsole API - Authorized Ssh Keys #1415

Closed

9 tasks

shiftkey self-assigned this Jul 6, 2016

Merge remote-tracking branch 'refs/remotes/octokit/master' into fix-t…

ff58f05

…imeout-getting-multiple-repositories

shiftkey reviewed Aug 1, 2016
View reviewed changes

remove some unused lines in httpclientadapter

802091c

Reworked redirect implementation to fully clone http request and re-u…

815f0d1

…se it later Now the skipped test from octokit#874 works! Also had to fix the new ReturnsRenamedRepository test as the ionide repo was renamed again

ryangribble merged commit 5b9e23c into octokit:master Aug 14, 2016

ryangribble changed the title ~~Fix timeout getting multiple repositories~~ Fix redirect timeout on renamed repositories Aug 14, 2016

grokys mentioned this pull request Sep 5, 2016

Update octokit to 0.22.0 github/VisualStudio#535

Merged

ryangribble mentioned this pull request Sep 15, 2016

Patch on redirected repo fails #874

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix redirect timeout on renamed repositories #1411

Fix redirect timeout on renamed repositories #1411

martinscholz83 commented Jul 5, 2016

ryangribble Jul 5, 2016

ryangribble commented Jul 5, 2016 •

edited

Loading

martinscholz83 commented Jul 5, 2016

shiftkey commented Jul 6, 2016

shiftkey commented Jul 11, 2016

martinscholz83 commented Jul 12, 2016

shiftkey commented Jul 12, 2016

martinscholz83 commented Jul 14, 2016

martinscholz83 commented Jul 15, 2016

ryangribble commented Jul 16, 2016

shiftkey Aug 1, 2016

shiftkey commented Aug 1, 2016

shiftkey Aug 1, 2016

shiftkey commented Aug 1, 2016

ryangribble commented Aug 1, 2016

shiftkey commented Aug 1, 2016

martinscholz83 commented Aug 1, 2016

shiftkey commented Aug 9, 2016

martinscholz83 commented Aug 9, 2016

alexander-efremov commented Aug 9, 2016 •

edited

Loading

martinscholz83 commented Aug 9, 2016 •

edited

Loading

ryangribble commented Aug 10, 2016 •

edited

Loading

martinscholz83 commented Aug 11, 2016

ryangribble commented Aug 11, 2016

martinscholz83 commented Aug 11, 2016

ryangribble commented Aug 12, 2016 •

edited

Loading

martinscholz83 commented Aug 13, 2016 •

edited

Loading

ryangribble commented Aug 14, 2016

ryangribble commented Aug 14, 2016

shiftkey commented Aug 15, 2016

martinscholz83 commented Aug 15, 2016

Fix redirect timeout on renamed repositories #1411

Fix redirect timeout on renamed repositories #1411

Conversation

martinscholz83 commented Jul 5, 2016

ryangribble Jul 5, 2016

Choose a reason for hiding this comment

ryangribble commented Jul 5, 2016 • edited Loading

martinscholz83 commented Jul 5, 2016

shiftkey commented Jul 6, 2016

shiftkey commented Jul 11, 2016

martinscholz83 commented Jul 12, 2016

shiftkey commented Jul 12, 2016

martinscholz83 commented Jul 14, 2016

martinscholz83 commented Jul 15, 2016

ryangribble commented Jul 16, 2016

shiftkey Aug 1, 2016

Choose a reason for hiding this comment

shiftkey commented Aug 1, 2016

shiftkey Aug 1, 2016

Choose a reason for hiding this comment

shiftkey commented Aug 1, 2016

ryangribble commented Aug 1, 2016

shiftkey commented Aug 1, 2016

martinscholz83 commented Aug 1, 2016

shiftkey commented Aug 9, 2016

martinscholz83 commented Aug 9, 2016

alexander-efremov commented Aug 9, 2016 • edited Loading

martinscholz83 commented Aug 9, 2016 • edited Loading

ryangribble commented Aug 10, 2016 • edited Loading

martinscholz83 commented Aug 11, 2016

ryangribble commented Aug 11, 2016

martinscholz83 commented Aug 11, 2016

ryangribble commented Aug 12, 2016 • edited Loading

martinscholz83 commented Aug 13, 2016 • edited Loading

ryangribble commented Aug 14, 2016

ryangribble commented Aug 14, 2016

shiftkey commented Aug 15, 2016

martinscholz83 commented Aug 15, 2016

ryangribble commented Jul 5, 2016 •

edited

Loading

alexander-efremov commented Aug 9, 2016 •

edited

Loading

martinscholz83 commented Aug 9, 2016 •

edited

Loading

ryangribble commented Aug 10, 2016 •

edited

Loading

ryangribble commented Aug 12, 2016 •

edited

Loading

martinscholz83 commented Aug 13, 2016 •

edited

Loading