Skip to content

Put the caller's transaction back in the params on end/rollback - #256

Merged
daffl merged 4 commits into
feathersjs:masterfrom
KidkArolis:fix-parent-transaction
Jul 13, 2021
Merged

Put the caller's transaction back in the params on end/rollback#256
daffl merged 4 commits into
feathersjs:masterfrom
KidkArolis:fix-parent-transaction

Conversation

@KidkArolis

@KidkArolis KidkArolis commented Nov 20, 2020

Copy link
Copy Markdown
Contributor

Not 100% confident this is correct, but here's my reasoning.

In my service.publish() publishers I want to await for transaction.promise, so that events only get emitted once the transaction, if any, is succesfully committed.

I can do that like this:

app.service('messages').publish((data, context) => {
  const { transaction } = context.params

  if (transaction) {
    try {
      await transaction.promise
    } catch (err) {
      return []
    }
  }

  return app.channel(`rooms/${data.roomId}`)
})

This works well if the service creates/updates other services in an after hook, forwarding the transcation as a param. The nested/related services will create things under that shared transaction, and will also await on the params.transaction.promise (in the publisher), before they send out the realtime events.

However, if the "nested" service also creates a transaction, that works well with respect to the DB queries, but in the transcation.end / transaction.rollback, it wipes the params.transaction, and the service loses the knowledge it was inside a transaction. So the event publisher can no longer await for that transaction.promise.

The idea is that any after hooks or event publishers inspecting params.transaction, should work the same regardless of whether the service has an intermediate transcation or not.

@KidkArolis

KidkArolis commented Dec 17, 2020

Copy link
Copy Markdown
Contributor Author

Hi! I've further developed this new capability - the ability to wait for transactions to complete. Previously, I was awaiting for transaction.promise, but recently realised that has nothing to do with knowing when the transaction has been committed / rejected, so my initial PR (and the issue description) were not correct.

I've since introduced a new separate promise onto a transaction for keeping track of when the transaction has been committed/rejected, that I also pass down to the child transactions.

Specifically, I use this in my own Feathers app and wanted to contribute this capability back.

I've now updated the code, added a basic test, and updated the README.

I'm open to suggestions on how to improve the naming/API, as I'm not 100% sure calling it committed is clear, and the way it resolves with true/false is the right approach. At first, I tried resolving for commit and rejecting for rollback, but was getting some unhandled promise warnings in Node, and instead of figuring out what's going on I opted for this resolve(success) approach. Again, open to suggestions and happy to make any adjustments.

And also, if you think this is too complicated / obscure, I'm happy to go for a different approach. The problems I'm trying to solve are:

  1. Avoid sending realtime events to the sockets in case transactions rollback.
  2. Avoid sending realtime events from nested calls before the parent services had a chance to commit, as this can lead to a client/server race condition where client receives events about resources that aren't yet visible in a separate request, since they're still getting committed.

@daffl
daffl merged commit f2a358f into feathersjs:master Jul 13, 2021
@daffl

daffl commented Jul 13, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Sorry for the long wait, I think this looks good, will go out with the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@KidkArolis @daffl