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

fix continous growth of app data in pooled requests #1609

Merged
merged 1 commit into from
Jul 18, 2020

Conversation

robjtede
Copy link
Member

@robjtede robjtede commented Jul 18, 2020

PR Type

What kind of change does this PR make?

Bug Fix

PR Checklist

Check your PR fulfills the following:

  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • A changelog entry has been made for the appropriate packages.

Overview

A change made in PR #1486 to support multiple layers of application data caused a leak of app data for each request object that was reclaimed by the request pool. This PR makes a simple change to set fresh app data for a re-used request object rather than appending to the existing vec of app data layers.

This PR also adds some documentation around the request pool so readers can better understand its purpose and life cycle.

fixes #1606
fixes #1607

@robjtede robjtede marked this pull request as ready for review July 18, 2020 03:48
@robjtede robjtede requested review from a team July 18, 2020 03:48
@codecov-commenter
Copy link

Codecov Report

Merging #1609 into master will decrease coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1609      +/-   ##
==========================================
- Coverage   52.94%   52.90%   -0.05%     
==========================================
  Files         124      124              
  Lines       11784    11773      -11     
==========================================
- Hits         6239     6228      -11     
  Misses       5545     5545              
Impacted Files Coverage Δ
src/app_service.rs 88.11% <100.00%> (ø)
actix-http/src/client/config.rs 88.88% <0.00%> (-11.12%) ⬇️
actix-http/src/encoding/encoder.rs 80.30% <0.00%> (-1.52%) ⬇️
actix-http/src/config.rs 58.71% <0.00%> (-0.92%) ⬇️
awc/src/sender.rs 25.89% <0.00%> (-0.90%) ⬇️
src/types/payload.rs 80.59% <0.00%> (-0.75%) ⬇️
actix-http/src/response.rs 40.80% <0.00%> (-0.34%) ⬇️
src/test.rs 88.58% <0.00%> (-0.28%) ⬇️
actix-http/src/h1/dispatcher.rs 56.53% <0.00%> (-0.26%) ⬇️
src/rmap.rs 94.15% <0.00%> (-0.04%) ⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2fd96c0...9b71f55. Read the comment docs.

Copy link
Member

@JohnTitor JohnTitor left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for fixing quickly!

@robjtede robjtede merged commit 971ba3e into master Jul 18, 2020
@robjtede robjtede deleted the fix/app-data-leak branch July 18, 2020 15:17
@attila-lin
Copy link

Nice work!
How about release a new version?

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.

Falling throughput when extracting path information Possible Memory Leak
4 participants