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.

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

[release/6.0] Enforce HttpClient limits on GetFromJsonAsync #80552

Merged
merged 3 commits into from
Feb 9, 2023

Conversation

MihaZupan
Copy link
Member

@MihaZupan MihaZupan commented Jan 12, 2023

Backport of a minimized change of #79386 to release/6.0

Customer Impact

HttpClient has two properties users can tweak to limit the amount of time and resources spent on a given request (Timeout and MaxResponseContentBufferSize).
GetFromJsonAsync is inconsistent in the enforcement of these limits compared to other helpers (GetStringAsync, GetByteArrayAsync, and DeleteFromJsonAsync).

There are three main ways to get the response content from HttpClient:

  1. Using the one-line helper methods
    await client.GetStringAsync("foo");            // Limits enforced
    await client.GetFromJsonAsync<MyClass>("foo"); // Limits **NOT** enforced
  2. Get the response object and call helpers on its content
    using HttpResponseMessage response = await client.SendAsync(request);
    await response.Content.ReadAsStringAsync();          // Limits enforced
    await response.Content.ReadFromJsonAsync<MyClass>(); // Limits enforced
  3. Optionally use ResponseHeadersRead, asking the client not to buffer the response content as part of the SendAsync call
    using HttpResponseMessage response = await client.SendAsync(request, HttpCompletionOption.ResponseHeadersRead);
    await response.Content.ReadAsStringAsync();          // Limits not enforced by design
    await response.Content.ReadFromJsonAsync<MyClass>(); // Limits not enforced by design

This PR changes the behavior of the client.GetFromJsonAsync helper to match that of GetStringAsync and friends (case 1).
This allows us to present consistent HttpClient behavior across the board.

Testing

I added targeted CI tests that confirm limits are consistently enforced.

Risk

The enforcement of limits means that some requests that would previously succeed may now fail (either time out or exceed the size limit). It is unlikely that anyone is knowingly relying on this behavior given the inconsistencies mentioned above.
The default limits are also very large (100 seconds and 2 GB of content), so for a request to hit them, the user has most likely lowered them manually, indicating the intent that they do want them to be enforced. It also means that if they do run into issues, they can tweak the existing settings directly.

The change can also result in slightly higher memory consumption as we're buffering the whole body before we start the deserialization process. We do not expect this to be meaningfully impactful.

@MihaZupan MihaZupan added this to the 6.0.x milestone Jan 12, 2023
@MihaZupan MihaZupan requested a review from a team January 12, 2023 15:07
@MihaZupan MihaZupan self-assigned this Jan 12, 2023
@ghost
Copy link

ghost commented Jan 12, 2023

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of a minimized change of #79386 to release/6.0

Customer Impact

TODO

Testing

TODO

Risk

TODO

Author: MihaZupan
Assignees: MihaZupan
Labels:

area-System.Net.Http

Milestone: 6.0.x

@carlossanlop
Copy link
Member

Last day to merge backports for the February Release is tomorrow. Please fill out the template, make sure to mention the customer impact. Add the servicing-release label, and send email to Tactics requesting approval.

@MihaZupan MihaZupan closed this Jan 13, 2023
@MihaZupan MihaZupan reopened this Jan 13, 2023
@carlossanlop carlossanlop added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jan 13, 2023
@carlossanlop
Copy link
Member

Talked to @MihaZupan. This will go in next month.

Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

Thanks!

@MihaZupan
Copy link
Member Author

Failures are #81391, #81544.

@MihaZupan MihaZupan added Servicing-consider Issue for next servicing release review and removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) labels Feb 8, 2023
@carlossanlop
Copy link
Member

Hey @ViktorHofer what should we do about this failure?:

.dotnet\sdk\6.0.113\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.EolTargetFrameworks.targets(28,5):
error NETSDK1138: (NETCORE_ENGINEERING_TELEMETRY=Build)
The target framework 'net5.0' is out of support and will not receive security updates in the future.
Please refer to https://aka.ms/dotnet-core-support for more information about the support policy.

@ViktorHofer
Copy link
Member

That's happening because of dotnet/sdk@4c7675d. Apparently serviced SDKs now also warn about a TFM that moves out of support, even when that's long after the stable SDK itself shipped originally.

We still produce net5.0 assets in our release/6.0 branch and we don't want to stop doing that. So we need an escape switch =>/p:CheckEolTargetFramework=false.

You can suppress that error by setting <CheckEolTargetFramework>false</CheckEolTargetFramework> here:

@karelz
Copy link
Member

karelz commented Feb 9, 2023

Approved by Tactics via email by @SteveMCarroll on 2/9.
Adding Servicing-approved label to the PR.

@karelz karelz added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Feb 9, 2023
@carlossanlop carlossanlop modified the milestones: 6.0.x, 6.0.15 Feb 9, 2023
@carlossanlop
Copy link
Member

Approved by Tactics for 6.0.15.
Signed-off by area owners.
CI failures unrelated and have fixes merged to the branch: #81544 #81391 #81848
Required OOB changes look good.
Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit 0e280c4 into dotnet:release/6.0 Feb 9, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Mar 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Http Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants