[iOS] HttpClient GetAsync does not consider connection loss as an error #5215
Comments
|
From @Flheriau on November 15, 2018 10:23 You can now reproduce with the sample project. I should have put it the first time ! |
|
From @kingces95 on November 16, 2018 23:35 Could you please provide a sample app as well as the URL used download an image that reproduces the problem? |
|
From @Flheriau on November 19, 2018 7:48 You can see a Sample project in the description. The image used is 10 Mo large to have the time to stop the internet connection. (https://www.spacetelescope.org/static/archives/images/publicationjpg/heic0711a.jpg) |
|
From @Flheriau on November 22, 2018 9:2 Any update on this ? Did you manage to reproduce the problem with the sample ? |
|
Hi, So I see some useful timeout info in the logs but, for the I see: If I'm using the managed HttpClient handler, I'm entering the catch and getting: |
VincentDondain
added
iOS
|
@VincentDondain there's another timeout issue in #5190 |
|
I tried to duplicate this before checking if the fix for #5190 had an impact. However it does seems to behave like I would expect, i.e. entering the It might already have been fixed inside the mono 2018 (since I'm using master) |
spouliot
added
the
need-info
|
I get similar results with current stable (12.2.1.12). @VincentDondain it's not clear which XI you used. Can you try again ? thanks! |
|
So with XI stable (a.k.a 12.2.1.12) I still get exactly the same behavior as I described here I tried with master @spouliot your iOS project is set to NSUrlSession for the HttpClient implementation right? Cause the catch is hit just fine with "Managed". Note: we don't hit the same exception. I tried with 2 different techniques: network conditioner 100% loss and turn off wifi (with not ethernet cable obviously). I'm killing the connection as soon as I hit the download button. |
@VincentDondain yes :) it's part of the stack trace (showing both the handler and that I'm getting the exception) |
|
@VincentDondain I actually met totally the same StackTrace of yours and the same 0x001c3 error code. |
|
@samhouts We have made some updates in the handler, I'll recheck the status of this issue. |
|
@mandel-macaque Do you have a link to the PR for those handler updates? We're being blocked by this issue as well. |
|
I have checked the current status of the given project, I tested the application with the following environment: https://gist.github.com/mandel-macaque/f9ce8bc4348f1d4e9dde000f677e7ead It looks like there is an issue with the Handler, which I used with an iPhon 8+ (iOS 12.1.4) resulting in no timeout exception, but with a partial image beings used:
@dotMorten I will work on a fix. If the network is lost (which I'm using via airplane mode) we should be getting an exception. The PRs I talked was #5190 but it is not throwing an exception. Will link here ASAP. |
mandel-macaque
removed
the
need-info
|
FYI... |
In some cases, when the connection is lost, the Stream is not set to have an exception and continues the read as normal. We need to ensure that the stream cannot be read when setting the response to an exception. We block the reading, set both exceptions and continue. In that case, the exception will be correctly raised when trying to read from the stream. A new check has to be done after the clean up of the stream if the session is using caching. Fixes xamarin#5215
|
@spouliot @VincentDondain it looks like we had an issue when setting the exception on both the response and the stream, which kind of explains why you could get different results. I have done a fair amount of testing to find out when this is happening and I believe the issue is as follows:
I believe that the fix would be to:
I've got a fix for it. I have tested it on device and on the simulator by using airplane mode on device or disconnecting the sim completely from the network. With the above reasoning, we always get the exception when disconnected even when using caching. With the fix we get an exception similar to the following:
|
|
The proposed change in commit 4e6241b should be fixing the issue and will ensure that you do a get TimeoutException (with the correct inner exception) in a consistent manner. The underlaying issue is the one I explained in my previous comment, changing the CancellationToken that is passed to the HttpCotent is a much safer bet than locking on the data read, which could still show some issue (less common, but I'm sure someone would hit it and reopen the issue) and a less async implementation. |
|
Oh! if TimeoutException is not the one wanted, please comment in the PR :) |
|
Well technically that doesn't sound like the right exception, but the main issue is an exception wasn't thrown and you assume the download is valid, so any exception is a million times better than no exception. Thank you for jumping on this! |
|
@dotMorten Please do comment in the PR about it. There are several options we can do about the exception, I was reading the CoreFx issues and they had a similar "discussion". In their case, users complained because they had a CancelationTaskException that was hard to identify (was the task cancelled via the CancellationToken in the SendAsync or was it due to another reason, ie Timeout). On our side, we have a diff problem but a similar results, after @spouliot PR we are getting a task cancelled when we reach the timeout, the reason is that there is a mismatch between the default timeout of the NSUrlSession and the HttpClient timeout. Due to that, we have a timeout BEFORE we get the correct NSError (stating that the connection was lost). @spouliot is correct in the sense that we want to respect the timeout, and technically, there is no real way I can propagate the NSError to the Stream of the HttpContent :/ Nevertheless, because we can control the exception raised, we could add a different one if it makes more sense. But as you said, we are getting the exception raised, which is what was truly needed. (I need to find the area of our docs in which I could write a small explanation for developers). |
…e. (#5769) In some cases, when the connection is lost, the Stream is not set to have an exception and continues the read as normal. We need to ensure that the stream cannot be read when setting the response to an exception. In order to do that, we use a new CancellationTokenSource whose token will be used for the HttpContent operations, therefore, if we received an error in the delegate, we can cancel the appropriate stream operations. A new check has to be done after the clean up of the stream if the session is using caching. Fixes #5215
|
In order to ensure that we will back port this from master to d16-1 could we please get some feedback from the recent build? Packages can be found in the following links. Your feedback is very welcome. |
|
Hi, |
|
Thanks for this! Huge. |
|
@mandel-macaque Thank you for the quick response! Greatly appreciated. Understood. I'll stand by. |
In some cases, when the connection is lost, the Stream is not set to have an exception and continues the read as normal. We need to ensure that the stream cannot be read when setting the response to an exception. We block the reading, set both exceptions and continue. In that case, the exception will be correctly raised when trying to read from the stream. A new check has to be done after the clean up of the stream if the session is using caching. Fixes xamarin#5215
|
So a few things:
|
|
@chamons Sounds good. Thanks! |
… reliable. (#5785) In some cases, when the connection is lost, the Stream is not set to have an exception and continues the read as normal. We need to ensure that the stream cannot be read when setting the response to an exception. In order to do that, we use a new CancellationTokenSource whose token will be used for the HttpContent operations, therefore, if we received an error in the delegate, we can cancel the appropriate stream operations. A new check has to be done after the clean up of the stream if the session is using caching. Fixes #5215
|
Replace NSUrl to Managed |
ghost
locked as resolved and limited conversation to collaborators
From @Flheriau on November 13, 2018 15:53
Description
I'm using HttpClient to download files from URLs with GetAsync method. With huge images for example, when I stop internet connection manually in the middle of the download, it is considered as a normal ending for the GetAsync method and the resulting HttpResponse is totally normal (status 200, no error message, no exception). Depending of the moment of the connection loss, the resulting image corresponds to the download progress (starting from the top of the image).
Steps to Reproduce
Expected Behavior
At least an exception for the connection loss or another status for the HttpResponse
Actual Behavior
Nothing.
Basic Information
Sample project
TestHttpClientGet.zip
Simple sample project with a "Download" button.
To reproduce, you need to stop internet connection after you clicked on Download.
Copied from original issue: xamarin/Xamarin.Forms#4392
The text was updated successfully, but these errors were encountered: