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

Connection.transaction: Reset Document#isNew if necessary when transaction is aborted #8852

Closed
SirUberKat opened this issue Apr 24, 2020 · 1 comment
Labels
new feature This change adds new functionality, like a new method or class
Milestone

Comments

@SirUberKat
Copy link

Do you want to request a feature or report a bug?
Question.

What is the current behavior?

I'm having a bit of hard time understanding the behaviour of the documents save method during transactions retries. As an transaction wrapper, I'm using mongoose-tx, only I had to modify it a bit by adding transaction abort if the database returned a TransientTransactionError :

        if (error.errorLabels && error.errorLabels.indexOf('TransientTransactionError') >= 0) {
            log.debug('TransientTransactionError, retrying transaction ...');
---->       await session.abortTransaction(); //Added here.
            await runTransactionWithRetry(txnFunc, session);
        } else {
            throw error;
        }

So the transaction itself is very simple:

await Tx(async session => {
      await doc1.save({ session });
      await doc2.save({ session });
})

And it works fine when the transaction does not get errors, however, if I get a TransientTransactionError due to a `WriteConflictz then things get weird.

So for the above example, doc2 is the one throwing the error and then two cases occur:

  1. If doc1 is a new document, then on the first transaction retry, it would throw a DocumentNotFound error and checking the isNew flag, I noticed that it was set to false even though the first transaction attempt failed (yes, await session.abortTransaction() was called.).

  2. If doc2 is not a new document, then on retry, it saves the document without errors, however due to the transaction error, it reverts any changes that were applied after retrieving it from the database.

What is the expected behavior?

I expected that abortTransaction would not revert the changes and for new documents it would reset the isNew flag. I don't know if this is the intended behaviour, but if it is, how do you properly handle retries with save ?

At the moment, as a workaround, I made a simple wrapper which either calls Model.create or Model.updateOne with modified paths instead of using save.

What are the versions of Node.js, Mongoose and MongoDB you are using? Note that "latest" is not a version.
Mongoose: 5.9.10
NodeJS: 12.15.0
MongoDB: 4.2.2

@vkarpov15 vkarpov15 added this to the 5.9.12 milestone Apr 27, 2020
@vkarpov15 vkarpov15 added the needs repro script Maybe a bug, but no repro script. The issue reporter should create a script that demos the issue label Apr 27, 2020
@vkarpov15 vkarpov15 modified the milestones: 5.9.12, 5.9.13, 5.9.14 May 4, 2020
@vkarpov15 vkarpov15 modified the milestones: 5.9.14, 5.9.15 May 13, 2020
@vkarpov15
Copy link
Collaborator

Confirmed that this is an issue. Right now the only workaround is for you to manually set isNew = true on every document you tried to save in the transaction.

What we'll do for the next minor release is add a mongoose.transaction() or db.transaction() function that resetting isNew for you. There are also several other features that we want to support for transactions, like #8380, that would be nice to add.

@vkarpov15 vkarpov15 modified the milestones: 5.9.15, 5.10 May 16, 2020
@vkarpov15 vkarpov15 added new feature This change adds new functionality, like a new method or class and removed needs repro script Maybe a bug, but no repro script. The issue reporter should create a script that demos the issue labels May 16, 2020
@vkarpov15 vkarpov15 changed the title Understanding save() behaviour in Transaction retries. Reset Document#isNew if necessary when transaction is aborted May 16, 2020
@vkarpov15 vkarpov15 changed the title Reset Document#isNew if necessary when transaction is aborted Connection.transaction: Reset Document#isNew if necessary when transaction is aborted Aug 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature This change adds new functionality, like a new method or class
Projects
None yet
Development

No branches or pull requests

3 participants