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

[query-frontend] Close response body in request handler #7654

Merged
merged 4 commits into from
Mar 19, 2024

Conversation

flxbk
Copy link
Contributor

@flxbk flxbk commented Mar 19, 2024

What this PR does

This closes the response body received by the frontend handler to free the resources associated with it. If this isn't done and the response.Body contains a non-noop io.ReadCloser it causes a memory leak.

Which issue(s) this PR fixes or relates to

Fixes #

Checklist

  • Tests updated.
  • Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • about-versioning.md updated with experimental features.

@flxbk flxbk marked this pull request as ready for review March 19, 2024 07:21
@flxbk flxbk requested a review from a team as a code owner March 19, 2024 07:21
@flxbk flxbk marked this pull request as draft March 19, 2024 07:37
@flxbk flxbk marked this pull request as ready for review March 19, 2024 07:58
CHANGELOG.md Outdated
@@ -13,6 +13,7 @@
* [BUGFIX] querier: Don't cache context.Canceled errors for bucket index. #7620
* [BUGFIX] Store-gateway: account for `"other"` time in LabelValues and LabelNames requests. #7622
* [BUGFIX] Query-frontend: Don't panic when using the `-query-frontend.downstream-url` flag. #7651
* [BUGFIX] Query-frontend: close the response body upon completion of the request. #7654
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] This likely won't make much sense to an end-user. Perhaps something like "fix memory leak on every request" would be more appropriate?

Copy link
Contributor Author

@flxbk flxbk Mar 19, 2024

Choose a reason for hiding this comment

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

Absolutely, 5d8e8b0

@flxbk flxbk changed the title [query-frontend] close response body in request handler [query-frontend] Close response body in request handler Mar 19, 2024
@flxbk flxbk merged commit eaef377 into main Mar 19, 2024
29 checks passed
@flxbk flxbk deleted the felix/frontend-leak branch March 19, 2024 10:42
@grafanabot
Copy link
Contributor

The backport to r280 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-7654-to-r280 origin/r280
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x eaef37753546273732aa7fc3e10fc670d57a4963
# Push it to GitHub
git push --set-upstream origin backport-7654-to-r280
git switch main
# Remove the local backport branch
git branch -D backport-7654-to-r280

Then, create a pull request where the base branch is r280 and the compare/head branch is backport-7654-to-r280.

@grafanabot
Copy link
Contributor

The backport to r279 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-7654-to-r279 origin/r279
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x eaef37753546273732aa7fc3e10fc670d57a4963
# Push it to GitHub
git push --set-upstream origin backport-7654-to-r279
git switch main
# Remove the local backport branch
git branch -D backport-7654-to-r279

Then, create a pull request where the base branch is r279 and the compare/head branch is backport-7654-to-r279.

@grafanabot
Copy link
Contributor

The backport to r281 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-7654-to-r281 origin/r281
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x eaef37753546273732aa7fc3e10fc670d57a4963
# Push it to GitHub
git push --set-upstream origin backport-7654-to-r281
git switch main
# Remove the local backport branch
git branch -D backport-7654-to-r281

Then, create a pull request where the base branch is r281 and the compare/head branch is backport-7654-to-r281.

@grafanabot
Copy link
Contributor

The backport to r282 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-7654-to-r282 origin/r282
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x eaef37753546273732aa7fc3e10fc670d57a4963
# Push it to GitHub
git push --set-upstream origin backport-7654-to-r282
git switch main
# Remove the local backport branch
git branch -D backport-7654-to-r282

Then, create a pull request where the base branch is r282 and the compare/head branch is backport-7654-to-r282.

flxbk added a commit that referenced this pull request Mar 19, 2024
* [query-frontend] close response body in request handler

* changelog

* check for nil

* changelog

(cherry picked from commit eaef377)
flxbk added a commit that referenced this pull request Mar 19, 2024
* [query-frontend] close response body in request handler

* changelog

* check for nil

* changelog

(cherry picked from commit eaef377)
flxbk added a commit that referenced this pull request Mar 19, 2024
* [query-frontend] Close response body in request handler (#7654)

* Initialize Tanka with 1.29. (#7544)

* Initialize Tanka with 1.29.

Signed-off-by: Peter Štibraný <[email protected]>

* Update other refs.

Signed-off-by: Peter Štibraný <[email protected]>

---------

Signed-off-by: Peter Štibraný <[email protected]>

---------

Signed-off-by: Peter Štibraný <[email protected]>
Co-authored-by: Peter Štibraný <[email protected]>
flxbk added a commit that referenced this pull request Mar 19, 2024
* [query-frontend] close response body in request handler

* changelog

* check for nil

* changelog

(cherry picked from commit eaef377)
flxbk added a commit that referenced this pull request Mar 19, 2024
* [query-frontend] close response body in request handler

(cherry picked from commit eaef377)
@grafanabot
Copy link
Contributor

The backport to release-2.12 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-7654-to-release-2.12 origin/release-2.12
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x eaef37753546273732aa7fc3e10fc670d57a4963
# Push it to GitHub
git push --set-upstream origin backport-7654-to-release-2.12
git switch main
# Remove the local backport branch
git branch -D backport-7654-to-release-2.12

Then, create a pull request where the base branch is release-2.12 and the compare/head branch is backport-7654-to-release-2.12.

flxbk added a commit that referenced this pull request Mar 19, 2024
* [query-frontend] close response body in request handler

(cherry picked from commit eaef377)
flxbk added a commit that referenced this pull request Mar 19, 2024
* [query-frontend] Close response body in request handler (#7654)

* [query-frontend] close response body in request handler

(cherry picked from commit eaef377)

* changelog
duricanikolic added a commit that referenced this pull request Mar 20, 2024
* Move -querier.minimize-ingester-requests from experimental to advanced (#7649)

Signed-off-by: Yuri Nikolic <[email protected]>

* Get rid of -querier.prefer-streaming-chunks-from-ingesters (#7639) (#7661)

* Get rid of querier.prefer-streaming-chunks-from-ingesters

Signed-off-by: Yuri Nikolic <[email protected]>

* Fixing a failing test

Signed-off-by: Yuri Nikolic <[email protected]>

* Fixing review findings

Signed-off-by: Yuri Nikolic <[email protected]>

* Fixing failing tests

Signed-off-by: Yuri Nikolic <[email protected]>

* Make lint happy

Signed-off-by: Yuri Nikolic <[email protected]>

* Fixing review findings

Signed-off-by: Yuri Nikolic <[email protected]>

---------

Signed-off-by: Yuri Nikolic <[email protected]>
(cherry picked from commit 8ed42e1)

Co-authored-by: Đurica Yuri Nikolić <[email protected]>

* [query-frontend] Close response body in request handler (#7654) (#7663)

* [query-frontend] Close response body in request handler (#7654)

* [query-frontend] close response body in request handler

(cherry picked from commit eaef377)

* changelog

* Update VERSIOn for release 2.12.0-rc.1 (#7671)

Signed-off-by: Yuri Nikolic <[email protected]>

---------

Signed-off-by: Yuri Nikolic <[email protected]>
Co-authored-by: Grot (@grafanabot) <[email protected]>
Co-authored-by: Felix Beuke <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants