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 recursion while waiting for redis connection #311

Merged
merged 3 commits into from
Aug 23, 2022

Conversation

nierob
Copy link
Contributor

@nierob nierob commented May 19, 2022

Arq has ability to retry connections to redis in case of problems.
That is a nice feature, but it crashes after hundreds of attempts
reaching maximal recursion depth. This change modifies re-try algorithm
from recursion to iteration avoiding the limit.

In practice it would be nice to have ability to wait forever as issue
with redis instance should not kill worker process, but that is a
separate change that can be built on top.

Fixes exception when retrying redis connection many times:
RecursionError: maximum recursion depth exceeded while getting the str of an object

@codecov
Copy link

codecov bot commented May 19, 2022

Codecov Report

Merging #311 (3ff25ad) into main (b4b40ad) will increase coverage by 0.10%.
The diff coverage is 100.00%.

❗ Current head 3ff25ad differs from pull request most recent head 2eab750. Consider uploading reports for the commit 2eab750 to get more accurate results

@@            Coverage Diff             @@
##             main     #311      +/-   ##
==========================================
+ Coverage   98.77%   98.87%   +0.10%     
==========================================
  Files          11       11              
  Lines         980      981       +1     
  Branches      183      183              
==========================================
+ Hits          968      970       +2     
  Misses          6        6              
+ Partials        6        5       -1     
Impacted Files Coverage Δ
arq/connections.py 95.55% <100.00%> (+0.03%) ⬆️
arq/worker.py 99.55% <0.00%> (+0.22%) ⬆️

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 b4b40ad...2eab750. Read the comment docs.

`Arq` has ability to retry connections to redis in case of problems.
That is a nice feature, but it crashes after hundreds of attempts
reaching maximal recursion depth. This change modifies re-try algorithm
from recursion to iteration avoiding the limit.

In practice it would be nice to have ability to wait forever as issue
with redis instance should not kill worker process, but that is a
separate change that can be built on top.

Fixes exception when retrying redis connection many times:
  RecursionError: maximum recursion depth exceeded while getting the str of an object
@nierob nierob force-pushed the fix_recursion_in_create_pool branch from 50bbdbf to f248aed Compare May 19, 2022 13:53
Copy link

@hermansc hermansc 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!

@nierob
Copy link
Contributor Author

nierob commented Jun 1, 2022

Hi, all the failures in CI feels unrelated, should I do something more to get this PR merged?

@samuelcolvin
Copy link
Owner

3.9 failure looks intermittent, might be worth rerunning.

If you can fix the problem with click that would be great, otherwise I will.

@nierob
Copy link
Contributor Author

nierob commented Jun 1, 2022

3.9 failure looks intermittent, might be worth rerunning.

I have no means of re-running other then changing something and re-pushing. I guess it is related to some permissions.

If you can fix the problem with click that would be great, otherwise I will.

#312

@samuelcolvin samuelcolvin enabled auto-merge (squash) August 23, 2022 16:36
@samuelcolvin
Copy link
Owner

samuelcolvin commented Aug 23, 2022

Thanks so much.

By the way, you can always re-run CI using git commit -m bump --allow-empty.

@samuelcolvin samuelcolvin merged commit 980d85e into samuelcolvin:main Aug 23, 2022
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.

None yet

3 participants