Use .NET standard HttpClient instead of HttpHelper #1326
Conversation
| return _httpClient; | ||
| } | ||
|
|
||
| var messageHandler = new HttpClientHandler() { MaxConnectionsPerServer = 10 }; |
There was a problem hiding this comment.
wouldn't we want to allow a developer to specify their own MaxConnectionsPerServer value if 10 didn't work for their scenario?
There was a problem hiding this comment.
@cbarkerms they can specify their own HttpMessageHandler which is then used when instantiating the httpClient.. we can do one or the other..
There was a problem hiding this comment.
ok, I see it now, thanks
| /// <returns>awaitable task</returns> | ||
| public virtual async Task InitializeAsync(StorageFolder folder, string folderName) | ||
| public virtual async Task InitializeAsync(StorageFolder folder, string folderName, HttpMessageHandler httpMessageHandler = null) |
There was a problem hiding this comment.
Can this method be simplified by calling the above overload?
There was a problem hiding this comment.
What the heck did i do that change for. Let me have another look there
| break; | ||
| } | ||
| } | ||
| catch (FileNotFoundException) |
There was a problem hiding this comment.
why are we swallowing exceptions?
There was a problem hiding this comment.
I have seen requests finish with no data.. Hence retry loop.. This happened on very rare occasions before as well but wasn't very noticable.. Swallowing only FileNotFound that is throw by InitializeAsync btw
| return _httpClient; | ||
| } | ||
|
|
||
| var messageHandler = new HttpClientHandler() { MaxConnectionsPerServer = 10 }; |
There was a problem hiding this comment.
ok, I see it now, thanks
Upgrade Microsoft.NETCore.UniversalWindowsPlatform nuget to 5.3.3 to see if appveyor issue is resolved
hermitdave
force-pushed
the
CacheBase-HttpClient
branch
from
96a9023 to
ceb3279
Compare
This change also maintains Microsoft.NETCore.UniversalWindowsPlatform NuGet package on version 5.3.3
| { | ||
| get | ||
| { | ||
| if (_httpClient != null) |
There was a problem hiding this comment.
Can we reverse this if
if(_httpClient == null)
{
var messageHandler = new HttpClientHandler() { MaxConnectionsPerServer = 10 };
_httpClient = new HttpClient(messageHandler);
}
return _httpClient;
| /// <param name="folderName">Cache folder name</param> | ||
| /// <param name="httpMessageHandler">instance of <see cref="HttpMessageHandler"/></param> | ||
| /// <returns>awaitable task</returns> | ||
| public virtual async Task InitializeAsync(StorageFolder folder, string folderName, HttpMessageHandler httpMessageHandler) |
There was a problem hiding this comment.
Why have all the overloads for this method? Why not one method with everything initialized to null?
public virtual async Task InitializeAsync(StorageFolder folder = null, string folderName = null, HttpMessageHandler httpMessageHandler = null)
There was a problem hiding this comment.
I personally hate having to set things to null when calling methods.. I am not fussed.. I can reverted to original calls only
There was a problem hiding this comment.
The dev wouldn't need to set anything because they already have defaults
There was a problem hiding this comment.
With the above method the dev can do the following:
InitializeAsync();
InitializeAsync(StorageFolder.GetFolder());
InitializeAsync("bob");
InitializeAsync(new ClientHttpMessageHandler());
InitializeAsync(StorageFolder.GetFolder(), "bob");
On and on...
| { | ||
| stream.CopyTo(fs); | ||
|
|
||
| fs.Flush(); |
There was a problem hiding this comment.
Shouldn't need to flush if in a using statement
There was a problem hiding this comment.
@skendrot I just encountered an issue when copying stream where stream needed flushing explicitly.. every single call resulted in partially copied stream returned to caller unless flush was called.
|
|
||
| webStream.Seek(0); | ||
| instance = await InitializeTypeAsync(randomAccessStream, initializerKeyValues).ConfigureAwait(false); |
There was a problem hiding this comment.
Can the stream from the response not be passed here?
There was a problem hiding this comment.
Bitmap requests IRandomAccessStream. The original stream is Stream and the operation has to be done somewhere.
Use .NET standard HttpClient instead of HttpHelper