Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Replace Webclient with HttpClient #18995

Merged
merged 2 commits into from
Mar 7, 2021
Merged

Replace Webclient with HttpClient #18995

merged 2 commits into from
Mar 7, 2021

Conversation

teinarss
Copy link
Contributor

@teinarss teinarss commented Jan 2, 2021

No description provided.

@pchote
Copy link
Member

pchote commented Jan 2, 2021

Depends on #18955 removing our dependency on my minimal mono repack (which doesn't have HttpClient).

phrohdoh
phrohdoh reviewed Jan 2, 2021
View reviewed changes
OpenRA.Game/Download.cs Outdated Show resolved Hide resolved
@Mailaender
Copy link
Member

No description provided.

Can you be a bit more elaborate on the upside of this change?

@teinarss
Copy link
Contributor Author

teinarss commented Jan 2, 2021

First of all its not recommended to use WebClient anymore, see https://docs.microsoft.com/en-us/dotnet/api/system.net.webclient?view=net-5.0#remarks
Also we should reuse HttpClient between different request, see https://aspnetmonsters.com/2016/08/2016-08-27-httpclientwrong/

@teinarss teinarss force-pushed the update_downloader branch 2 times, most recently from 9bd8d2f to 1d0457f Compare January 11, 2021 17:59
@teinarss teinarss marked this pull request as ready for review January 11, 2021 18:29
@teinarss teinarss mentioned this pull request Feb 19, 2021
Closed
@pchote
Copy link
Member

pchote commented Feb 20, 2021

Needs a rebase

penev92
penev92 requested changes Feb 21, 2021
View reviewed changes
Copy link
Member

@penev92 penev92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of my comments are naming or stylistic nitpicking. From what I tested only the news download in MainMenuLogic isn't working and I left a comment on what is missing.
I still do not understand why HttpClientFactory (or its consumers?) isn't caching anything, though, since I was left with the impression that it might be a good idea to reuse one or more instances of HttpClient instead of creating a new one for every request. Please elaborate.

OpenRA.Game/Download.cs Outdated Show resolved Hide resolved
OpenRA.Game/Download.cs Outdated Show resolved Hide resolved
OpenRA.Game/Download.cs Outdated Show resolved Hide resolved
OpenRA.Game/Download.cs Outdated Show resolved Hide resolved
OpenRA.Game/Download.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Widgets/Logic/Installation/DownloadPackageLogic.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Widgets/Logic/Installation/DownloadPackageLogic.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Widgets/Logic/MainMenuLogic.cs Show resolved Hide resolved
OpenRA.Mods.Common/Widgets/Logic/MainMenuLogic.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Widgets/Logic/MainMenuLogic.cs Outdated Show resolved Hide resolved
@teinarss teinarss force-pushed the update_downloader branch 2 times, most recently from ae75ded to ab2b21c Compare February 21, 2021 13:22
@teinarss
Copy link
Contributor Author

updated and rebased.

penev92
penev92 previously approved these changes Feb 21, 2021
View reviewed changes
Copy link
Member

@penev92 penev92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good and I couldn't spot regressions. 👍

OpenRA.Game/HttpExtension.cs Outdated Show resolved Hide resolved
OpenRA.Game/HttpExtension.cs Show resolved Hide resolved
@teinarss
Copy link
Contributor Author

updated.

pchote
pchote reviewed Feb 21, 2021
View reviewed changes
OpenRA.Game/Support/HttpQueryBuilder.cs
builder.Append("?");

foreach (var parameter in parameters)
builder.Append($"{parameter.Name}={parameter.Value}&");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this leave a stray & at the end of the query string?
Do we care?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW it doesn't seem to hurt 🤷

@teinarss
Copy link
Contributor Author

Updated.

@pchote pchote merged commit 7073279 into OpenRA:bleed Mar 7, 2021
@pchote
Copy link
Member

pchote commented Mar 7, 2021

https://github.com/OpenRA/OpenRA/wiki/Changelog-(bleed)/_compare/f64167602cac66408161c2c2e5ae0b8741a36663

@pchote pchote mentioned this pull request Mar 9, 2021
Merged
@pchote pchote mentioned this pull request Apr 6, 2021
Merged
@teinarss teinarss deleted the update_downloader branch April 15, 2021 16:59
@pchote pchote mentioned this pull request Apr 15, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phrohdoh phrohdoh phrohdoh left review comments

@penev92 penev92 penev92 left review comments

@pchote pchote pchote approved these changes

Assignees
No one assigned
Labels
PR: Needs +2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@teinarss @pchote @Mailaender @phrohdoh @penev92