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

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

Merged
merged 4 commits into from
Jul 13, 2021

Conversation

KidkArolis
Copy link
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
Copy link
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-ecosystem:master Jul 13, 2021
@daffl
Copy link
Member

daffl commented Jul 13, 2021

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants