-
Notifications
You must be signed in to change notification settings - Fork 451
Support celerycam for reconnecting db connenction if db connection is lost accidentally #550
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds automatic reconnection logic for Celery snapshot state handling when the Django database connection is lost.
- Imports
connection
andInterfaceError
from Django. - Wraps worker and task handling in a
try/except
to catch a closed connection. - Closes the connection and logs a message so Django’s connection pooling can reconnect.
Comments suppressed due to low confidence (1)
djcelery/snapshot.py:141
- Ensure that a
logger
instance is defined or imported in this module before usinglogger.info()
, otherwise this will raise aNameError
at runtime.
logger.info(
djcelery/snapshot.py
Outdated
except InterfaceError as e: | ||
# When connection already closed exception is raised, | ||
# force to close connection and Django will automatically reconnect | ||
if str(e) == 'connection already closed': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comparing exception message strings is brittle; consider catching a more specific exception subclass or inspecting error attributes instead of matching on str(e)
.
if str(e) == 'connection already closed': | |
if getattr(e, 'args', None) and 'connection already closed' in e.args[0]: |
Copilot uses AI. Check for mistakes.
djcelery/snapshot.py
Outdated
try: | ||
for worker in state.workers.items(): | ||
self.handle_worker(worker) | ||
_handle_tasks() | ||
except InterfaceError as e: | ||
# When connection already closed exception is raised, | ||
# force to close connection and Django will automatically reconnect | ||
if str(e) == 'connection already closed': | ||
connection.close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The try/except
block wraps both worker and task handling, which may swallow unrelated errors. Consider narrowing the scope to only the database operations that need reconnection.
try: | |
for worker in state.workers.items(): | |
self.handle_worker(worker) | |
_handle_tasks() | |
except InterfaceError as e: | |
# When connection already closed exception is raised, | |
# force to close connection and Django will automatically reconnect | |
if str(e) == 'connection already closed': | |
connection.close() | |
for worker in state.workers.items(): | |
self.handle_worker(worker) | |
_handle_tasks() | |
try: | |
# Check and handle database reconnection if needed | |
connection.close() | |
except InterfaceError as e: | |
# When connection already closed exception is raised, | |
# force to close connection and Django will automatically reconnect | |
if str(e) == 'connection already closed': |
Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds automatic reconnection logic for celerycam when the Django database connection is lost.
- Imports
connection
andInterfaceError
to detect closed connections - Wraps worker/task handling in a try-except to catch
InterfaceError
and force-close the connection - Logs when the connection is closed so Django can reconnect on next use
Comments suppressed due to low confidence (1)
djcelery/snapshot.py:136
- Add a unit or integration test that simulates
InterfaceError
during worker/task handling to verify the reconnection logic runs correctly and doesn’t break the loop.
except InterfaceError as e:
try: | ||
for worker in state.workers.items(): | ||
self.handle_worker(worker) | ||
_handle_tasks() | ||
except InterfaceError as e: | ||
# When connection already closed exception is raised, | ||
# force to close connection and Django will automatically reconnect | ||
if getattr(e, 'args', None) and 'connection already closed' in e.args[0]: | ||
connection.close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The try
block currently wraps both worker and task loops, which may swallow unrelated InterfaceError
s. Consider narrowing its scope to only the database-close operation or isolating the catch to the specific call that can raise due to a closed connection.
try: | |
for worker in state.workers.items(): | |
self.handle_worker(worker) | |
_handle_tasks() | |
except InterfaceError as e: | |
# When connection already closed exception is raised, | |
# force to close connection and Django will automatically reconnect | |
if getattr(e, 'args', None) and 'connection already closed' in e.args[0]: | |
connection.close() | |
for worker in state.workers.items(): | |
self.handle_worker(worker) | |
_handle_tasks() | |
try: | |
# Force to close connection if "connection already closed" exception is raised | |
connection.close() | |
except InterfaceError as e: | |
if getattr(e, 'args', None) and 'connection already closed' in e.args[0]: |
Copilot uses AI. Check for mistakes.
# When connection already closed exception is raised, | ||
# force to close connection and Django will automatically reconnect | ||
if getattr(e, 'args', None) and 'connection already closed' in e.args[0]: | ||
connection.close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of calling connection.close()
directly, use django.db.close_old_connections()
to properly handle connection pooling and ensure thread safety according to Django's recommendations.
connection.close() | |
close_old_connections() |
Copilot uses AI. Check for mistakes.
Currently if the database is not stable and connection is lost, celerycam can not reconnect db connection automatically and it can not work properly anymore. The error
connection already closed
will always be raised.This PR supports to automatically reconnect db connection for celerycam if db connection is lost.