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

Save a reference to created async tasks, to avoid tasks potentially disappearing #2816

Merged
merged 4 commits into from
Sep 21, 2023

Conversation

anio
Copy link
Contributor

@anio anio commented Jun 28, 2023

Save a reference to the result of this function, to avoid a task disappearing mid-execution. The event loop only keeps weak references to tasks. A task that isn’t referenced elsewhere may get garbage collected at any time, even before it’s done.

python docs

Pull Request check-list

Please make sure to review and check all of these items:

  • Does $ tox pass with this change (including linting)?
  • Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Is there an example added to the examples folder (if applicable)?
  • Was the change added to CHANGES file?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Description of change

Please review the link: https://textual.textualize.io/blog/2023/02/11/the-heisenbug-lurking-in-your-async-code/

@codecov-commenter
Copy link

codecov-commenter commented Jul 16, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01% ⚠️

Comparison is base (012f7cf) 91.38% compared to head (a7b45d7) 91.37%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2816      +/-   ##
==========================================
- Coverage   91.38%   91.37%   -0.01%     
==========================================
  Files         126      126              
  Lines       32469    32469              
==========================================
- Hits        29671    29669       -2     
- Misses       2798     2800       +2     
Files Changed Coverage Δ
redis/asyncio/cluster.py 91.66% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@chayim chayim changed the title save a reference to created tasks, to avoid tasks disappearing Save a reference to created async tasks, to avoid tasks potentially disappearing Sep 20, 2023
@chayim chayim added the maintenance Maintenance (CI, Releases, etc) label Sep 20, 2023
redis/asyncio/cluster.py Outdated Show resolved Hide resolved
redis/asyncio/cluster.py Outdated Show resolved Hide resolved
@dvora-h dvora-h merged commit 56b254e into redis:master Sep 21, 2023
55 checks passed
@jakob-keller
Copy link
Contributor

@anio: Thanks for bringing the issue up!

I am not convinced that the fix is appropriate, though. As the source you provide mentions:

The solution recommended in the docs is to keep a reference to the task for as long as you need it to live.

The way I understand it is, that we need to save the task reference beyond the scope of the local / loop variable. As it stands, the reference will be lost almost immediately, rendering the fix ineffective.

SebbyLaw added a commit to SebbyLaw/redis-py that referenced this pull request Oct 10, 2023
SebbyLaw added a commit to SebbyLaw/redis-py that referenced this pull request Jan 9, 2024
SebbyLaw added a commit to SebbyLaw/redis-py that referenced this pull request Feb 14, 2024
SebbyLaw added a commit to SebbyLaw/redis-py that referenced this pull request Apr 14, 2024
SebbyLaw added a commit to SebbyLaw/redis-py that referenced this pull request Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Maintenance (CI, Releases, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants