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/7.0] Enforce HttpClient limits on GetFromJsonAsync #80553

Merged
merged 2 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/7.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 change 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 7.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/7.0

Customer Impact

TODO

Testing

TODO

Risk

TODO

Author: MihaZupan
Assignees: MihaZupan
Labels:

area-System.Net.Http

Milestone: 7.0.x

@carlossanlop
Copy link
Member

Tomorrow is the last day for merging backports for the February Release. Can you please fill out the template, making sure the customer impact is clearly described, add the servicing-consider label, and send an email to Tactics requesting approval?

Also, there are networking-related CI failures. Can you please investigate them?

@MihaZupan
Copy link
Member Author

Build failure is known according to build analysis.

@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 MihaZupan added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) 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 6, 2023
@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: 7.0.x, 7.0.4 Feb 9, 2023
@carlossanlop
Copy link
Member

Approved by Tactics for 7.0.4.
Signed-off by area owners.
CI failure was a timeout cancellation in staging.
Required OOB package changes look good in System.Net.Http.Json.
Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit 3c73365 into dotnet:release/7.0 Feb 9, 2023
@carlossanlop
Copy link
Member

carlossanlop commented Feb 9, 2023

@MihaZupan @ManickaP I'm looking at the rolling builds for the release/7.0 branch, and after merging this PR, I am seeing a nuget issue related to System.Net.Http.Json:

https://github.com/dotnet/runtime/runs/11233952875

Can you please take a look? I'll open an issue to track this. It needs to get fixed before Monday EOD. That's the day we close the servicing branches.

Edit: I opened #81914 and pinged Viktor/Eric for help.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 12, 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.

4 participants