Transaction doesn't rollback

[emorris00]

Version: 0.0.11

The "Database entity is not set for the given subject" error does not rollback transactions

// this will remove row with id 1 but not id 2
repository.transaction(async repository=> {
    repository.remove({id: 1})
    repository.remove({}) // errors out here, but also commits transaction
    repository.remove({id: 2})
})

[pleerock]

You need to await pending promises:

// this will remove row with id 1 but not id 2
repository.transaction(async repository=> {
    await repository.remove({id: 1})
    await repository.remove({}) // errors out here, but also commits transaction
    await repository.remove({id: 2})
})

Also I strongly recommend you to update orm version to 0.1.0

[emorris00]

Ah sorry, apparently I didn't understand my problem. It appears that the problem is with using Promise.all. Let me know what you think. This is using version 0.1.0

import {createConnection, getConnection, Connection, Entity, PrimaryColumn} from 'typeorm'


@Entity()
class TestEntity {
    @PrimaryColumn({type: "int"})
    id: number
}

// transaction should throw and not delete the row
// passes
async function test1() {
    let connection = getConnection()
    await connection.manager.save(TestEntity, {id: 1})

    console.assert(await connection.manager.findOneById(TestEntity, 1), "row doesn't exist before transaction")
    
    let didThrow = false
    try {
        await connection.transaction(async manager => {
            await manager.remove(TestEntity, {id: 1})
            await new Promise((resolve, reject) => reject('reject'))
        })
    }
    catch(e) {
        didThrow = true
    }

    console.assert(didThrow, "transaction didn't throw")
    console.assert(await connection.manager.findOneById(TestEntity, 1), "row doesn't exist after transaction")
}

// transaction should throw and not delete the row
// fails, row gets deleted
async function test2() {
    let connection = getConnection()
    await connection.manager.save(TestEntity, {id: 1})

    console.assert(await connection.manager.findOneById(TestEntity, 1), "row doesn't exist before transaction")

    let didThrow = false
    try {
        await connection.transaction(async manager => {
            await Promise.all([
                manager.remove(TestEntity, {id: 1}),
                new Promise((resolve, reject) => reject('reject')),
            ])
        })
    }
    catch(e) {
        didThrow = true
    }

    console.assert(didThrow, "transaction didn't throw")
    console.assert(await connection.manager.findOneById(TestEntity, 1), "row doesn't exist after transaction")
}


async function runTest(func: Function) {
    try {
        await func()
        console.log(`${func.name} passed`)
    }
    catch(e) {
        console.log(`${func.name} failed,`, e.message)
    }
}


createConnection({
    type: "mysql",
    host: "localhost",
    port: 3306,
    username: "",
    password: "",
    database: "test",
    entities: [TestEntity],
    synchronize: true,
}).then(async connection => {
    await runTest(test1)
    await runTest(test2)
    process.exit()
})

[pleerock]

can you simplify your code and point me to exact line where your problem is?

[emorris00]

// this throws an error, but the entity still gets removed
await connection.transaction(async manager => {
    await Promise.all([
        manager.remove(TestEntity, {id: 1}),
        new Promise((resolve, reject) => reject('reject')),
    ])
})

[pleerock]

I can reproduce this issue.

However, its how promises are working and its because of how Promise.all works.
The problem is that when promise.all executes it does not wait for all promises to be resolved and throws error once any of promise rejects but other promises are executed at this moment, after rejection has made. ORM listens to rejects and executed rollback. But since there is a promise in execution at the same moment, DELETE is executed anyway.

Solution is to combine for loop and await, or simply run promises in order without using Promise.all. You can also use PromiseUtils.runInSequence.

You need something like bluebird's settle function. I just added it into PromiseUtils class internally used in orm. Example:

    /**
     * Returns a promise that is fulfilled with an array of promise state snapshots,
     * but only after all the original promises have settled, i.e. become either fulfilled or rejected.
     */
    function settle(promises: Promise<any>[]) {
        return Promise.all(promises.map(p => Promise.resolve(p).then(v => ({
            state: "fulfilled",
            value: v,
        }), r => ({
            state: "rejected",
            reason: r,
        })))).then((results: any[]): any => {
            const rejected = results.find(result => result.state === "rejected");
            if (rejected)
                return Promise.reject(rejected.reason);

            return results.map(result => result.value);
        });
    }

    // --------------------

     await connection.transaction(manager => {
                await settle([
                    manager.remove(TestEntity, { id: 1 }),
                    new Promise((resolve, reject) => reject(new Error())),
                ]);
      });

More detailed answer to the problem and solution is here.

[teyou]

await [manager.remove(TestEntity, {id: 1}), 
       new Promise((resolve, reject) => reject('reject'))
       ].reduce((prev, cur) => prev.then(cur), Promise.resolve());

another way of running Promise in a sequence

[emorris00]

This is just an idea, but would it be possible to have the query runners store references to the promises from queries they run if its in a transaction, and then change rollbackTransaction to wait for all of them to resolve before rolling back?

[pleerock]

QueryRunner does not work with promises, it just accepts a single promise. Its unlikely possible what you are saying because even if we do crazy transaction method which accepts multiple arguments - functions that return multiple promises there can be lot of nested promises inside them which will have same issue.

[emorris00]

I'm not sure we are talking about the same thing, let me give you a rough example of what I mean.

class MysqlQueryRunner {
	promises = []

	query(query: string, parameters?: any[]): Promise<any> {
		if (this.isReleased)
			throw new QueryRunnerAlreadyReleasedError();

		let promise = new Promise(async (ok, fail) => {
			// ...
		});

                // store the promise from query if runner is in a transaction
		if(this.isTransactionActive) {
			this.promises.push(promise);
		}

		return promise
	}


	async rollbackTransaction(): Promise<void> {
		if (!this.isTransactionActive)
			throw new TransactionNotStartedError();

                // wait for all stored promises to resolve before rolling back
		await settle(this.promises);
		await this.query("ROLLBACK");
		this.isTransactionActive = false;
		this.promises = [];
	}
}

[pleerock]

its not scalable solution at all because you are covering only Promise.all on first level, we aren't you consider about Promise.all inside Promise.all ?

[pleerock]

I think this can be closed.

[calvintwr]

Hi, just ran into this problem. Has it changed? I noticed that in other ORMs, there isn’t this problem.

Is the solution then to handle transaction manually — wrap it in try/catch, and call rollback in catch?

[emorris00]

I think you should be able to create a promise that runs Promise.allSettled and rejects if any of the promises in Promise.allSettled rejected.