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

Retry on "server closed connection unexpectedly" errors #259

Merged
merged 10 commits into from
Jun 18, 2020
Merged

Conversation

elemoine
Copy link
Contributor

@elemoine elemoine commented Jun 17, 2020

Closes #255.

Successful PR Checklist:

  • Tests
    • (not applicable?)
  • Documentation
    • (not applicable?)
  • Had a good time contributing?
  • (Maintainers: add PR labels)

@codecov
Copy link

codecov bot commented Jun 17, 2020

Codecov Report

Merging #259 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            master      #259    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           25        25            
  Lines         1228      1448   +220     
  Branches       140       186    +46     
==========================================
+ Hits          1228      1448   +220     
Impacted Files Coverage Δ
procrastinate/aiopg_connector.py 100.00% <100.00%> (ø)
procrastinate/psycopg2_connector.py 100.00% <100.00%> (ø)
procrastinate/app.py 100.00% <0.00%> (ø)
procrastinate/cli.py 100.00% <0.00%> (ø)
procrastinate/worker.py 100.00% <0.00%> (ø)

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 dff2f6a...a4fd08a. Read the comment docs.

@elemoine elemoine force-pushed the ele_255 branch 3 times, most recently from b0c1b96 to e360db4 Compare June 17, 2020 15:36
@elemoine
Copy link
Contributor Author

Now with tests.

@ewjoachim
Copy link
Member

ewjoachim commented Jun 17, 2020

What does it do with a Psycopg2 sync connector ?

@elemoine
Copy link
Contributor Author

What does it do with a Psycopg2 sync connector ?

You're right here to. We need to make the same sort of changes to the Psycopg2 connector.

@elemoine elemoine force-pushed the ele_255 branch 2 times, most recently from 44073c4 to 888a4af Compare June 17, 2020 17:27
@elemoine elemoine force-pushed the ele_255 branch 2 times, most recently from f018d3a to 161c857 Compare June 18, 2020 06:38
@elemoine
Copy link
Contributor Author

@ewjoachim I think I took all your comments into account. But happy to have more comments if you have any :)

Comment on lines 54 to 56
nonlocal cnt
if cnt < 2:
cnt += 1
Copy link
Member

Choose a reason for hiding this comment

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

A more natural way to do this is to declare a list outside the function, and append things inside.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@elemoine elemoine force-pushed the ele_255 branch 3 times, most recently from 5f5c999 to 5d970b3 Compare June 18, 2020 10:14
connection.rollback()
raise
else:
connection.commit()
Copy link
Member

Choose a reason for hiding this comment

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

If we put the putconn here, then we don't need the outer try anymore, no ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, and I also need to call putconn after rollback.

Changed.

@ewjoachim
Copy link
Member

We're getting there \o/ Thanks for your continued efforts.

@elemoine
Copy link
Contributor Author

I am done updating the PR. Ready for final review.

@elemoine elemoine merged commit cd87e67 into master Jun 18, 2020
@elemoine elemoine deleted the ele_255 branch June 18, 2020 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR type: feature ⭐️ Contains new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle closed connections gracefully
2 participants