DownloadStreamAsync does not handle 404 pages?

[kendallb]

DO NOT USE ISSUES FOR QUESTIONS

Describe the bug
As it stands currently, DownloadStreamAsync executes the request and blindly downloads whatever response it gets back regardless of whether the request was successful on or. So if you have someone rendering a fancy 404 page like this one:

http://bti-usa.com/images/pictures/sf/sf3058.jpg

It gets happily downloaded as a file (which of course is not actually a jpg file, but that is not the root issue here :) ).

To Reproduce
Download the URL above using DownloadStreamAsync and it will download the contents.

Expected behavior
I think the correct behavior here is that DownloadStreamAsync should check for a valid HTTP status (say 200/OK?) and then return null or something if the status is not valid?

I am not sure if that would be a breaking change or not, as it is not clear what the callers would expect for the resulting stream on error. Perhaps it would expect an exception to be thrown if something was wrong, otherwise it expects a valid stream and would not expect a null (and would crash with an NRE).

For now I am simply going to implement my own download code that handles the response codes, but I think this is something that the library should be responsible for doing correctly?

[alexeyzimarev]

Yeah, that's a miss. I will add a test, and then we can decide what to do with it. If the client is set to throw, I can accommodate that. However, 404 isn't currently considered as an error. Maybe returning nothing would be a good solution?

[kendallb]

I already have a fix in my own branch, so you could pull that over for the 404 handling? Also added an exception when we attempt to use a disposed RestClient :)

kendallb@27c4766

[alexeyzimarev]

I think adding this would be too big of a change, as it doesn't work this way now

        if (!responseMessage.IsSuccessStatusCode) {
            var statusCode = responseMessage.StatusCode;
            throw new WebException($"The remote server returned an error: ({(int)statusCode}) {statusCode}");
        }

What I can do is to move the check downstream, particularly to the Download function. I am not sure yet if it's doable.

The default behaviour was always that RestSharp doesn't throw by default and rather return an error response. For example, you can examine the response content to find out what it says. The client throws in GetAsync etc overloads, it's by design, or when the client ThrowOnAnyError is set. As the download function is relatively new, I can change its behaviour without breaking too much, but it still needs to be a major version update.

[kendallb]

Well I am not sure you can move it any higher up, as the function I am calling is DownloadStreamAsync so for my use case it is already as high up the food chain as possible.

So while it is not how it functions today, it is also impossible to implement the mechanism of returning an Exception from that function because there is no return response from DownloadStream or Download to inspect. We could change it to only throw that exception if Options.ThrowOnAnyError is set so it would be the same as the exception handling above it, but I do wonder if that option makes any sense at all inside DownloadStreamAsync because there is no way for the caller to ever get the underlying exception that occurred if Options.ThrowOnAnyError is false? It will just silently fail and you get really strange results downloaded.

[kendallb]

Looks good