Patch on redirected repo fails #874
Comments
|
Seems to be something odd ... If I run from VS, it seems to time out on the send of the new request (following the redirect). Yet, I run Fiddler, and it all works. Disable Fiddler Traffic Capture and it times out. Re-enable and it works. |
|
@shiftkey Any thoughts on this? Do you see the same problem? |
|
@Red-Folder digging my way out from under a heap of psychic debt at the moment, so i'm not quite sure when I'll get to investigating this. But thanks for reporting it! |
|
I can confirm I'm seeing the same thing as @Red-Folder, running the test in VS fails with a timeout. Start Fiddler, then run the test in VS and the test passes. I'll see if I can dig a bit deeper and figure out what is going on. |
|
Bumping this and opening it up for investigating, because I've seen it while testing #989 |
|
@shiftkey Sorry - I didn't get much further in my investigation than noted above. I seem to remember that I felt it was something to do with a connection being held open and it causing blocking (maybe some reference not being released correctly) - but it is likely to be something quite low level if the problem "disappears" when Fiddler is involved. I have no scientific proof for my theory ;) I'll have a another go when I get a chance (maybe wireshark or something similar will be less obtrusive than Fiddler and give some indication if the connection is still open) |
|
@Red-Folder it's all good. I wish I had a better answer for you about what might be happening but I've got many Windmills™ to Tilt At™* at the moment... * citation |
|
Ok, update on progress. I'm using an old version of the code (not pulled for about 3 months) ... but can recreate the problem for the Octokit.Tests.Integration.RedirectTests.CanCreateIssueOnRedirectedRepository test. As originally, if I run the test from VS, it hangs/ times out. Yes if I run with Fiddler, then I have no problem. So I've run with Wireshark ... I've included a summary below (good luck to anyone that can understand it). From what I guess, it appears to have 2 TSL requests (which we'd expect) the handshakes, setup etc all seem fine. However with both, api.github.com are providing in FIN/ ACK - which is closing the connection. Then some time later, we get the RST/ ACK from the client (for some reason I have my laptop set up as www.x.com) - this seems to be the timeout. I wonder if something is going wrong with the HTTPClient not correctly handling the FIN/ ACK. Equally, I could be heading down the wrong rabbit hole. Wireshark summary: No. Time Source Destination Protocol Length Info |
|
I'm seeing this too, including the odd behavior that it works when Fiddler is open. I thought I was going crazy. It's happening for me on calls to |
…se it later Now the skipped test from octokit#874 works!
…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
…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
…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
…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
* add test * [WIP] * put logic for redirects outside of delegating handler * change send method * format code * reorganized http client adapter * change HttpClientAdapter * rework http redirect tests - still an issue with accessing the response.RequestMessage.Content property as it is disposed * remove some unused lines in httpclientadapter * Reworked redirect implementation to fully clone http request and re-use it later Now the skipped test from #874 works! Also had to fix the new ReturnsRenamedRepository test as the ionide repo was renamed again
|
This is a bit of a drive-by ... spotted this article about the use of HttpClient -> http://aspnetmonsters.com/2016/08/2016-08-27-httpclientwrong/ This feels as if it may be related to this problem. Not had a chance to run through the code (and not likely to in the near future) ... but figured I'd throw this in while passing. Hope it helps. |
|
Thanks for the bump and reference, this has actually been fixed by changes in #1411 and the failing test has been unskipped, looks like I forgot to come back and close this 😀 |
|
@ryangribble Excellent news. Great stuff |
nickfloyd
added
Type: Bug
This is being picked up by the integration test: Octokit.Tests.Integration.RedirectTests.CanCreateIssueOnRedirectedRepository
The test runs until it tries to update the issue.
From stepping through:
It looks like the new request doesn't have its content set-up correctly.
I'll have more of a dig when I get a chance, but wanted to post while still fresh in my head.
The text was updated successfully, but these errors were encountered: