New url session handler #932
Conversation
This is a rewrite of the ModernHttpClient version of NSUrlSessionHandler, it has better handling for memory that provides a more consistant memory footprint. It accomplishes this by using NSInputStream for requests, and reading and disposing directly from NSData instead of transitioning the NSData to a byte[] array.
Mostly my comments in PR #174 * Add support for redirection [1] * Add support for credentials [1] * Add support for caching [2] * Remove 2nd dictionary lookup in GetHeaderSeparator * Avoid extraneous cast for credentialsToUse PR 177 [3] adds tests that ensure no commits can remove, or change default values, for handlers. [1] breaking changes (feature, not API) [2] breaking change (API removal) [3] #177
Mostly my comments in PR #174 * Add support for redirection [1] * Add support for credentials [1] * Add support for caching [2] * Remove 2nd dictionary lookup in GetHeaderSeparator * Avoid extraneous cast for credentialsToUse PR 177 [3] adds tests that ensure no commits can remove, or change default values, for handlers. [1] breaking changes (feature, not API) [2] breaking change (API removal) [3] #177
* Added ConfigureAwait(false) to Task.Delay to prevent DEADLOCK when the stream is being awaited on the UI thread
|
Build failure |
|
|
||
| // we cannot do a bitmask but we can set the minimum based on ServicePointManager.SecurityProtocol minimum | ||
| var sp = ServicePointManager.SecurityProtocol; | ||
| if ((sp & SecurityProtocolType.Ssl3) != 0) |
There was a problem hiding this comment.
CC @spouliot
This defaults to the least secure option (Ssl3), maybe we should reverse the conditions here to put TLS 1.2 first?
There was a problem hiding this comment.
@rolfbjarne I'll double check (later, this can/must go in now @mandel-macaque) but IIRC that's the minimum you want to support (as the client) but it's the server side that has the final say on the protocol version to be used. Setting Tls12 would as the minimum would break all TLS 1.1 (and older) sites.
| #endif | ||
| public partial class NSUrlSessionHandler : HttpMessageHandler | ||
| { | ||
| private readonly Dictionary<string, string> headerSeparators = new Dictionary<string, string> { |
There was a problem hiding this comment.
Style: we don't use private when it's the default.
|
|
||
| protected override void Dispose (bool disposing) | ||
| { | ||
| lock(dataLock) { |
There was a problem hiding this comment.
Style: space before opening parenthesis.
| } | ||
| } | ||
|
|
||
| await Task.Delay(50).ConfigureAwait(false); |
There was a problem hiding this comment.
Style: space before opening parenthesis.
|
Build failure |
|
Build failure |
|
@spouliot Maybe it's a bit late to suggest this, but maybe we should provide the old NSUrlSessionHandler as well, so that customers can keep using it if there are bugs with this one? Once this one has been thoroughly tested (in a few cycles), we could remove the old one. |
|
@rolfbjarne it's a refactoring of the existing one, i.e. it's pretty close, except for one important part of reusing the allocated memory, and ModernHttpClient remains a 3rd party component that can be used. |
|
Build success |
|
@mandel-macaque I'm curious about this line: Why is the fallback cache policy not taken from the configuration that was parsed into the constructor instead? |
This PR is built on top of the following two PRs:
As per the first PR:
With the original ModernHttpClient version of NSUrlSessionHandler it had 3 problems.
This pull request does the following the correct the above.
Please note this is a complete rewrite, which is why I didn't try to commit it to the ModernHttpClient repository. I am willing to move it some where else if it is more appropriate.
The only work done has been to fix the merge issues and ensure that we are ok with master. QA has already approved the merge.