Skip to content

Only clear the cache if there are no active writers.#7273

Merged
auvipy merged 1 commit intomasterfrom
asynpool-memory-leak-fix-regression
Aug 1, 2022
Merged

Only clear the cache if there are no active writers.#7273
auvipy merged 1 commit intomasterfrom
asynpool-memory-leak-fix-regression

Conversation

@naomielst
Copy link
Contributor

In #6863 we discarded all jobs if synack isn't enabled for the pool.
This fixed a severe memory leak which occurs on connection restart.

Instead of going over each job and checking if we should discard it, we should clear the entire cache when there are no active writers.

If there are active writers, we should discard the jobs from the cache after we're done writing them since they also may remain on the cache forever.

Note: Before submitting this pull request, please review our contributing
guidelines
.

Description

In #6863 we discarded all jobs if synack isn't enabled for the pool.
This fixed a severe memory leak which occurs on connection restart.

Instead of going over each job and checking if we should discard it, we should clear the entire cache when there are no active writers.

If there are active writers, we should discard the jobs from the cache after we're done writing them since they also may remain on the cache forever.
@codecov
Copy link

codecov bot commented Feb 1, 2022

Codecov Report

Merging #7273 (4c8dcde) into master (25ca389) will increase coverage by 1.69%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #7273      +/-   ##
==========================================
+ Coverage   89.31%   91.01%   +1.69%     
==========================================
  Files         138      138              
  Lines       16774    19637    +2863     
  Branches     2450     3384     +934     
==========================================
+ Hits        14982    17872    +2890     
+ Misses       1560     1525      -35     
- Partials      232      240       +8     
Flag Coverage Δ
unittests 90.99% <ø> (+1.68%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
celery/fixups/django.py 93.08% <0.00%> (-0.95%) ⬇️
celery/worker/request.py 96.61% <0.00%> (-0.08%) ⬇️
celery/contrib/rdb.py 100.00% <0.00%> (ø)
celery/contrib/abortable.py 100.00% <0.00%> (ø)
celery/app/amqp.py 94.83% <0.00%> (+0.08%) ⬆️
celery/schedules.py 83.62% <0.00%> (+0.08%) ⬆️
celery/backends/asynchronous.py 69.26% <0.00%> (+0.43%) ⬆️
celery/security/__init__.py 94.44% <0.00%> (+0.69%) ⬆️
celery/worker/consumer/gossip.py 96.96% <0.00%> (+0.75%) ⬆️
celery/backends/filesystem.py 95.06% <0.00%> (+0.77%) ⬆️
... and 26 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 58873e3...4c8dcde. Read the comment docs.

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

should we try to add some tests or increase some test coverage?

@auvipy auvipy added this to the 5.2.x milestone Mar 6, 2022
@auvipy auvipy modified the milestones: 5.2.x, 5.3 Jun 7, 2022
@auvipy auvipy closed this Aug 1, 2022
@auvipy auvipy reopened this Aug 1, 2022
@auvipy auvipy merged commit 0aeac3d into master Aug 1, 2022
@Nusnus Nusnus deleted the asynpool-memory-leak-fix-regression branch July 23, 2024 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants