Handle SystemExit gracefully in two phase commit mode #21

sjustas · 2020-12-23T13:28:15Z

If SystemExit was raised while PJDataManager was joining a transaction in two-phase-commit mode, PJDataManager failed to clean up after itself.

Traceback (most recent call last):
  File "/usr/lib/python3.8/unittest/case.py", line 60, in testPartExecutor
    yield
  File "/usr/lib/python3.8/unittest/case.py", line 679, in run
    self._callTearDown()
  File "/usr/lib/python3.8/unittest/case.py", line 636, in _callTearDown
    self.tearDown()
  File "/home/justas/src/shoobx/pjpersist/src/pjpersist/tests/test_datamanager.py", line 1649, in tearDown
    super(TwoPhaseCommitTestCase, self).tearDown()
  File "/home/justas/src/shoobx/pjpersist/src/pjpersist/testing.py", line 220, in tearDown
    transaction.abort()
  File "/home/justas/src/shoobx/pjpersist/ve/lib/python3.8/site-packages/transaction/_manager.py", line 260, in abort
    return self.manager.abort()
  File "/home/justas/src/shoobx/pjpersist/ve/lib/python3.8/site-packages/transaction/_manager.py", line 139, in abort
    return self.get().abort()
  File "/home/justas/src/shoobx/pjpersist/ve/lib/python3.8/site-packages/transaction/_transaction.py", line 570, in abort
    reraise(t, v, tb)
  File "/home/justas/src/shoobx/pjpersist/ve/lib/python3.8/site-packages/transaction/_compat.py", line 49, in reraise
    raise value
  File "/home/justas/src/shoobx/pjpersist/ve/lib/python3.8/site-packages/transaction/_transaction.py", line 551, in abort
    rm.abort(self)
  File "/home/justas/src/shoobx/pjpersist/src/pjpersist/datamanager.py", line 790, in abort
    self._conn.rollback()
psycopg2.ProgrammingError: rollback cannot be used during a two-phase transaction

coveralls · 2020-12-23T13:30:35Z

Coverage increased (+0.09%) to 86.161% when pulling 4e6b59d on handle_system_exit into 2eb7aef on master.

kedder · 2020-12-23T13:32:04Z

src/pjpersist/datamanager.py

-        self._tpc_activated = True
+        finally:
+            if self._conn.status == psycopg2.extensions.STATUS_BEGIN:
+                self._tpc_activated = True


I don't understand what this have to do with SystemExit.

If SystemExit is raised in just the right time, _tpc_activated flag was not being set (because of exception) even though tpc_begin had just completed. So when PJDataManager tried to clean up, it called _conn.rollback() - this is invalid call for a connection in status STATUS_BEGIN. And so it failed with ProgrammingError

SystemExit is just an exception we encountered in production, when a celery worker is being shut down while still trying to process some database task.

You mean you think this happens when SystemExit is raised right between tpc_begin() and _tpc_activated = True?

I see, this makes sense. but the code needs in addition:

Comment, explaining why this code is there

Logging message on warning level when we don't set _tpc_activated to True.

@kedder re-arranged the code a bit and added logging. Hope it reads better now.

kedder · 2020-12-23T13:57:31Z

src/pjpersist/datamanager.py

-        self._tpc_activated = True
+        finally:
+            if self._conn.status == psycopg2.extensions.STATUS_BEGIN:
+                self._tpc_activated = True


I see, this makes sense. but the code needs in addition:

Comment, explaining why this code is there

Logging message on warning level when we don't set _tpc_activated to True.

kedder

Looks good

agroszer · 2020-12-23T16:34:21Z

src/pjpersist/datamanager.py

+                    "PJDataManager._begin: tried to call connection.tpc_begin,"
+                    " but it did not change connection status to STATUS_BEGIN,"
+                    " failing with a generic exception %s."
+                    "  Not setting TPC mode on PJDataManager." % exc)


nitpicking: , exc please instead of %

…ining a transaction in two-phase-commit mode.

kedder reviewed Dec 23, 2020

View reviewed changes

strichter approved these changes Dec 23, 2020

View reviewed changes

kedder requested changes Dec 23, 2020

View reviewed changes

sjustas force-pushed the handle_system_exit branch from 80efc38 to 05317e0 Compare December 23, 2020 14:25

kedder approved these changes Dec 23, 2020

View reviewed changes

agroszer reviewed Dec 23, 2020

View reviewed changes

Handle SystemExit gracefully if it's raised while PJDataManager is jo…

4e6b59d

…ining a transaction in two-phase-commit mode.

sjustas force-pushed the handle_system_exit branch from 05317e0 to 4e6b59d Compare December 28, 2020 15:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle SystemExit gracefully in two phase commit mode #21

Handle SystemExit gracefully in two phase commit mode #21

sjustas commented Dec 23, 2020

coveralls commented Dec 23, 2020 •

edited

Loading

kedder Dec 23, 2020

sjustas Dec 23, 2020

sjustas Dec 23, 2020

kedder Dec 23, 2020

sjustas Dec 23, 2020

kedder Dec 23, 2020

sjustas Dec 23, 2020

kedder Dec 23, 2020

kedder left a comment

agroszer Dec 23, 2020

Handle SystemExit gracefully in two phase commit mode #21

Are you sure you want to change the base?

Handle SystemExit gracefully in two phase commit mode #21

Conversation

sjustas commented Dec 23, 2020

coveralls commented Dec 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kedder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 23, 2020 •

edited

Loading