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: refactor transactions to use their own event loops #443

Merged
merged 4 commits into from
May 22, 2020

Conversation

chrisrossi
Copy link
Contributor

Referring to issue #426, the problem ultimately turned out to be that
we could fall out of the transaction scope and trigger a commit while
there is still work left queued on the event loop, including, in this
case, the tasklet that would eventually schedule the call to delete,
causing the delete to never actually happen.

The fix is to go ahead and consume the event loop queues before
scheduling the call to COMMIT. However, if there are other tasks
happening in parallel, this can really mess with the natural sequence of
events in ways that can cause things to blow up. (All of the
parallel_transaction tests in tests/system/test_misc.py for
instance, will fail.) The fix for that is to give each transaction its
own event loop, so that when it calls _eventloop.run prior to commit,
it is only flushing tasks that pertain to it.

Fixes #426

Referring to issue googleapis#426, the problem ultimately turned out to be that
we could fall out of the transaction scope and trigger a commit while
there is still work left queued on the event loop, including, in this
case, the tasklet that would eventually schedule the call to delete,
causing the delete to never actually happen.

The fix is to go ahead and consume the event loop queues before
scheduling the call to COMMIT. However, if there are other tasks
happening in parallel, this can really mess with the natural sequence of
events in ways that can cause things to blow up. (All of the
`parallel_transaction` tests in `tests/system/test_misc.py` for
instance, will fail.) The fix for that is to give each transaction its
own event loop, so that when it calls `_eventloop.run` prior to commit,
it is only flushing tasks that pertain to it.

Fixes googleapis#426
@chrisrossi chrisrossi requested a review from cguardia May 22, 2020 14:52
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 22, 2020
@chrisrossi
Copy link
Contributor Author

chrisrossi commented May 22, 2020

Following up on the main commit message, the move to separate event loops for transactions is not without its own complications. Sometimes flow of control will get passed back to the main loop before the transaction callback, if it is a tasklet, has finished. In this case, the main loop needs to be able to poke the inner loops to keep doing work until they've finished. This is handled by adding an idle handler to the main loop, run_inner_loop, that runs one item from the inner loop and reschedules itself, or if there is no work to do, doesn't reschedule itself.

There is also a minor change in the way we're handling deciding whether to call AllocateIds. We only need to call AllocateIds if some piece of code has inserted a new entity and has asked for the returned key value before our call to Commit. If no one ever asks for the key, we don't need to call AllocateIds and can let the id be allocated during the Commit call itself. We use an idle handler to Allocate the ids, since idle handlers generally only get called when someone is waiting on something we don't have yet. Previously, if we got to Commit before the idle handler got run, we'd just take the ids that needed to be allocated out of the idle handlers queue, so there wouldn't be anything for it to do if it called after Commit. Now, since we're flushing the event loop before calling Commit, the idle will always be called before the commit, but if we're flushing the queues to prepare for a commit, we don't need to allocate ids, because we're about to call Commit. So now we call _datastore_api.prepare_to_commit right before flushing the event loop, which sets a flag that signals the idle handler that we don't need to allocate ids.

@@ -396,13 +395,7 @@ def run():
loop.run()


def run0():
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wasn't used.

@@ -123,7 +123,10 @@ def wait(self):
after a call to this method.
"""
while not self._done:
_eventloop.run1()
if not _eventloop.run1():
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sanity check.

Copy link
Contributor

@cguardia cguardia left a comment

Choose a reason for hiding this comment

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

Wow, this took time to parse. Looks great.

@chrisrossi
Copy link
Contributor Author

Pretty much took all week to figure out! Thanks!

@chrisrossi chrisrossi merged commit 7590be8 into googleapis:master May 22, 2020
@chrisrossi chrisrossi deleted the fix-426 branch May 22, 2020 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

transactional deletes (at least) misbehaves with Redis
3 participants