Check the status code, honor the encoding and reuse the HttpClient when downloading - #9
Merged
Merged
Conversation
…nt when downloading Behavior change - DownloadToString threw nothing for an error status and returned the content of the error page. TryDownloadToString therefore reported success for a 404. It now throws an HttpRequestException (as DefaultDownloader already did), so TryDownloadToString returns false. Fixes - DownloadToString ignored its encoding parameter. An explicit encoding is now used; without one the charset announced by the server is used (UTF-8 if there is none), which is what happened before. A byte order mark in the content takes precedence in both cases. - HttpChannelDownloader passed UTF-8 as the encoding when none was given. It passes null now, so the server charset keeps being used by default and only an explicit encoding is forced. Performance - DefaultDownloader and DownloaderWithCredentials created a new HttpClient for every download, which exhausts sockets and is slow. DefaultDownloader shares one client, DownloaderWithCredentials creates its client once per instance. - the shared clients recycle their connections every two minutes (SocketsHttpHandler; not available on netstandard2.0), so that a long-lived client notices changed DNS entries - new constructor DefaultDownloader(HttpClient) to configure the client (proxy, timeout, ...); the client is not disposed by the downloader. The parameterless constructor is unchanged. Tests use a fake server (StubHandler) or a local HttpListener with basic authentication; the helpers are shared with DownloadHeaderTest. The IDownloader documentation describes the behavior.
Review findings for the shared HttpClient. Cookies (P1) - the shared client of the downloaders used a handler with cookies enabled. A session cookie set for one download was sent with later downloads of other, independent downloaders, so sessions of different users or jobs could get mixed. Before, every download had its own client and was isolated. - clients created by DefaultHttpClient do not store cookies by default. This includes the process-wide HttpChannelExt.SharedHttpClient, which had the same problem before this change (a shared static client with a cookie container). Stateful downloads stay possible with a client the caller passes in (DefaultDownloader(HttpClient)) or by assigning HttpChannelExt.SharedHttpClient. - DefaultDownloader and HttpChannelExt.SharedHttpClient start with the same client (DefaultHttpClient.Shared) and so share one connection pool Exceptions - the synchronous wrappers used .Result, which wraps every failure in an AggregateException, while EnsureSuccessStatusCode threw the HttpRequestException directly. They use GetAwaiter().GetResult() now, so callers always get the HttpRequestException itself. Other - DownloaderWithCredentials requires its credentials (ArgumentNullException) - fix a typo in the IDownloader documentation Tests - DownloaderCookieTest: two independent downloaders, repeated downloads, DownloaderWithCredentials and the default IHttpChannel do not share cookies; a client the caller owns may keep them - LocalHttpServer: Dispose called Stop() and then Close(). The managed HttpListener then removes the prefix a second time, which fails with "Address already in use" while pooled connections of the client are open (about 1 in 20 runs). Close() alone is enough. Ports are also handed out only once per process and a busy port is retried.
Review findings. - the documentation claimed that a byte order mark takes precedence in every case. That is only true with an explicit encoding (StreamReader detects the mark). Without one, HttpContent.ReadAsStringAsync decides: it uses the charset the server announces and ignores a mark of another encoding (UTF-8 content with a mark and "charset=iso-8859-1" is decoded as Latin-1). This path is unchanged from before, so the docs are corrected and not the code - DownloadHeader disposes its request like DownloadToString does - IDownloader docs: the fallback of TryDownloadToString is "" and not default; note that netstandard2.0 cannot recycle connections, so hosts there that follow DNS changes should pass their own client Tests: an explicit encoding loses against a byte order mark; a client passed to DefaultDownloader keeps its timeout; an already disposed client fails when it is used.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The remaining network items from the phase 4 plan, plus the findings of two reviews. Three of the changes alter behavior, please read that part.
Behavior changes
DownloadToStringreturned the content of the error page for a 404 or 500, soTryDownloadToStringreported success for a missing resource. It now throws anHttpRequestException, andTryDownloadToStringreturnsfalse.AggregateException. The synchronous wrappers used.Result, which wraps every failure (404 inDefaultDownloader, no connection, ...) in anAggregateException. They useGetAwaiter().GetResult()now, so callers get theHttpRequestExceptionitself, and all downloaders behave the same. Code that catchesAggregateExceptionaround these calls has to catchHttpRequestExceptioninstead.DefaultDownloaderandDownloaderWithCredentials: a per-download client is replaced by a shared one, and a cookie container on a shared client would mix the sessions of independent users and jobs. (Review finding, P1: reproduced with a local server, the cookie of one downloader was sent by another.)HttpChannelExt.SharedHttpClient, which theIHttpChannelextension methods use, is a process-wide static client that always had a cookie container, so it had this problem before this PR. It is fixed too. Anyone relying on cookies being kept betweenchannel.DownloadToStringcalls has to assign a client withUseCookies = truetoHttpChannelExt.SharedHttpClient, or useDefaultDownloader(HttpClient)with a client of their own.If the release should stay free of behavior changes until 13.0, the three points are the ones to hold back; the rest does not depend on them.
Fixes
DownloadToStringignored itsencodingparameter. An explicit encoding is used now, but a byte order mark in the content still wins over it. Without an explicit encoding nothing changes:HttpContent.ReadAsStringAsyncuses the charset the server announces (UTF-8, or the encoding of a byte order mark, if there is none) and does not look at a byte order mark when a charset is announced.HttpChannelDownloaderalways passed UTF-8 when no encoding was given. Honoring the parameter alone would have forced UTF-8 for servers that announce another charset, so it passesnullnow and only an explicit encoding is forced.DownloaderWithCredentialsrequires its credentials (ArgumentNullException).Performance
DefaultDownloaderandDownloaderWithCredentialscreated a newHttpClientfor every download (socket exhaustion, slow).DefaultDownloaderandHttpChannelExt.SharedHttpClientnow start with the same client (one connection pool),DownloaderWithCredentialscreates its client once per instance.DefaultDownloader(HttpClient)to configure the client (proxy, timeout, cookies, ...). The client is not disposed by the downloader. The parameterless constructor is unchanged.SocketsHttpHandlerthat recycles connections every two minutes, so a long-lived client notices changed DNS entries.netstandard2.0has noSocketsHttpHandler, so it uses a plainHttpClientHandlerthere (first#if NETSTANDARD2_0in the code base).The public API only grows (one constructor). The
IDownloaderdocumentation describes the behavior.Not part of this PR
IAsync*interfaces.DownloadHeaderstill does not check the status code on purpose: many servers answer HEAD with 405 and the headers are still useful.Tests
DownloadToStringTest(fake server): error status, explicit and server encoding, byte order mark (also against an explicit encoding),HttpChannelDownloaderwith and without an encodingDefaultDownloaderTest: the given client is used, keeps its timeout and is not disposed, exceptions are not wrapped, credentials are required,DownloaderWithCredentialssends the credentials and can be used repeatedly (local server with basic authentication)DownloaderCookieTest(local server): independent downloaders, repeated downloads,DownloaderWithCredentialsand the defaultIHttpChanneldo not share cookies; a client the caller owns may keep themLocalHttpServerhad a flaw that made about 1 in 20 runs fail:DisposecalledStop()and thenClose(). The managedHttpListenerthen removes the prefix a second time, which fails with "Address already in use" while pooled connections are open.Close()alone is enough.Verification
net10.0andnet8.0(local, macOS); 200 runs of the local-server tests and 100 full runs without a failureCap.Core 12.0.0: nothing removed (only the knownISpanFormattablenoise from droppingnet6.0)