Transactional not applying on whole method ?

[jchapelle]

When trying this library, I realize that the transaction is committed after the first "await this.service1.create" while I would expect the transaction to be committed at the end of the method.

@Transactional()
    async myMethod(id, message): Promise<Post> {
        const service1Result = await this.service1.create({ id, message })  // which makes a repository1.save()
        const service1Result = await this.service2.create({ id, message })  //  which makes a repository2.save()
        runOnTransactionCommit(() => this.events.emit('post created'))
        return result
}

[matiasraisedreal]

I have a similar error and it is that although the rollback and commit are executed well. The 'save' is committed before ending the function, so the subscription in this case is saving in the DB.

 @Transactional()
  async createSubscription(subscription: SubscriptionDto): Promise<Subscription> {

    runOnTransactionCommit(() => console.log('subscription created'));
    runOnTransactionRollback(() => console.log('subscription roll back'));

    const sub = await this.repository.save(subscription);

    if (subscription.userId === 1) throw 'fail';

    return sub;
}

[odavid]

I think the save method should be awaited...

…

On Thu, 12 Nov 2020 at 17:38 matiasraisedreal ***@***.***> wrote: I have a similar error and it is that although the rollback and commit are executed well. The 'save' is committed before ending the function, so the subscription in this case is saving in the DB. @transactional() async createSubscription(subscription: SubscriptionDto): Promise<Subscription> { runOnTransactionCommit(() => console.log('subscription created')); runOnTransactionRollback(() => console.log('subscription roll back')); const sub = this.repository.save(subscription); if (subscription.userId === 1) throw 'fail'; return sub; } — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#58 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGSOAP4ODR3J3VQKQZ4WNLSPP6Q5ANCNFSM4TAW73GA> .

[odavid]

I think the save method should be awaited...
…

Sorry, ignore that...

[odavid]

@matiasraisedreal - can you try using Constructor instead of create()

[odavid]

@matiasraisedreal - can you try using Constructor instead of create()

Also, are the service create methods also decorated with transactional?

[matiasraisedreal]

Thank you very much @odavid for the quick reply.
I didn't quite understand what your suggestion is. Maybe I'll share some code to give you a better context for the comment ...

Just to understand the scope of the library. Does it rollBack the transactions in the database that occur within the function defined with the transactional () decorator? Or does it only provide you with a function ("runOnTransactionRollback"), which can be called if at some point the function / transaction fails?

// subscription.controller.ts

@ApiTags('Subscriptions')
@Controller('subscriptions')
export class SubscriptionController {
  constructor(private readonly subscriptionService: SubscriptionService) {}

  @Post()
  @Public()
  createSubscription(
    @Body() subscription: SubscriptionDto
  ): Promise<Subscription | SubscriptionDto> {
    return this.subscriptionService.createSubscription(subscription);
  }
}

subscription.service.ts

@Injectable()
export class SubscriptionService {
  constructor(readonly repository: SubscriptionRepository) { }

  @Transactional()
  async createSubscription(subscription: SubscriptionDto): Promise<Subscription | SubscriptionDto> {

    runOnTransactionCommit(() => console.log('subscription created'));
    runOnTransactionRollback(() => console.log('subscription roll back'));

    const sub = this.repository.save(subscription);

    if (subscription.userId === 1) throw 'fallo';

    return sub;
  }
}

subscription.repository.ts

@EntityRepository(Subscription)
export class SubscriptionRepository extends BaseRepository<Subscription> {}

[matiasraisedreal]

@odavid It would be of great help and I would be very grateful if you could give me any suggestions based on what I shared with you 😉

[odavid]

Hey @mathiswiehl - I think the issue you have is because you don't await the save method... so it is running later on
Can you try to change the code and see it happens?

@Injectable()
export class SubscriptionService {
  constructor(readonly repository: SubscriptionRepository) { }

  @Transactional()
  async createSubscription(subscription: SubscriptionDto): Promise<Subscription | SubscriptionDto> {

    runOnTransactionCommit(() => console.log('subscription created'));
    runOnTransactionRollback(() => console.log('subscription roll back'));

    const sub = await this.repository.save(subscription);

    if (subscription.userId === 1) throw 'fallo';

    return sub;
  }
}

[matiasraisedreal]

Hi @odavid !
I just added the await, but it's the same. The subscription is created without problems and however the console still throws me the error and the function "runOnTransactionRollback ()" is executed. Attached image.

I feel that the Repository 'save' function is not "listening" to what is happening, as if it were disconnected from what the library is.
I tried to downgrade typeOrm to version 0.2.22, but the same thing keeps happening...

[odavid]

@matiasraisedreal - Will it be possible to create a simple test that I can clone and reproduce?

[odavid]

Hi @matiasraisedreal , I've created a small test using postgresql and I could not reproduce the issue you have.
Can u please clone this repository and then run:

yarn install
yarn test

What is the database engine you're using?

[sunjoong85]

It's not safe to use Transactional() in a loop and complex async/await calls.

Unhandled async logic can be executed even though your IDE or lint doesn't recognize it as an unhandled Promises.

public async mainApi() {
        const promiseMemo = [];  // Keep promises 
        const revenues = await this.revenueMgmtRepository.find();

        revenues.forEach((revenue) => {
              promiseMemo.push(this.revenueMgmtRepository.update(revenue));
              
              ... revenue assertion logic. And it throws somewhere in this loop.

             // Not safe. because it has two async logic inside. @Transactional doesn't wait until all the async calls are done.
              promiseMemo.push(this.secondApi(revenue));   

        })

       await Promise.all(promiseMemo);   // Not called, when an exception occured in a loop.
  }

  private async secondApi(revenue) {
    const foundAccount = await this.hostAccountMgmtService.findAccountByHost(entity.hostId);

    revenue.setAccount(foundAccount);

    await this.revenueTxMgmtRepository.insert(rtx);  // This could be called out side of the transactional scope.
  }

Conclusion,

It's not safe to use @transactional in a complex situation.

I am not sure that I can manage the transactional scope per method or per request like Spring Transaction does in any Node frameworks.

I have to accept that asynchronous calls are too complicated to be handled.

[jchapelle]

Thank you all for the investigations. From my point of view, a proper transaction management mechanism is the main missing feature and limitation of nodejs/typeorm. Without that mechanism, it is almost impossible to develop secure and complex logic with database interactions. PS : I'm not experienced enough to try to develop one ;-)

…

On Mon, Nov 23, 2020 at 8:14 AM Sunjoong Kim ***@***.***> wrote: Same issue. It's not safe to use Transactional() in a loop and complex async/await calls. Unhandled async logic can be executed even though your IDE or lint doesn't recognize it as an unhandled Promises. public async mainApi() { const promiseMemo = []; // Keep promises const revenues = await this.revenueMgmtRepository.find(); revenues.forEach((revenue) => { promiseMemo.push(this.revenueMgmtRepository.update(revenue)); ... revenue assertion logic. And it throws somewhere in this loop. // Not safe. because it has two async logic inside. @transactional doesn't wait until all the async calls are done. promiseMemo.push(this.secondApi(revenue)); }) await Promise.all(promiseMemo); // Not called, when an exception occured in a loop. } private async secondApi(revenue) { const foundAccount = await this.hostAccountMgmtService.findAccountByHost(entity.hostId); revenue.setAccount(foundAccount); await this.revenueTxMgmtRepository.insert(rtx); // This could be called out side of the transactional scope. } Conclusion, It's not safe to use @transactional <https://github.com/transactional> in a complex situation. I am not sure that I can manage the transactional scope per method or per request like Spring Transaction does in any Node frameworks. I have to accept that asynchronous calls are too complicated to be handled. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#58 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABPAMXIOR5MKL3IIJT6VPE3SRIDU3ANCNFSM4TAW73GA> .

-- Jean Chapelle

[sunjoong85]

It's not safe to use Transactional() in a loop and complex async/await calls.

Unhandled async logic can be executed even though your IDE or lint doesn't recognize it as an unhandled Promises.

public async mainApi() {
        const promiseMemo = [];  // Keep promises 
        const revenues = await this.revenueMgmtRepository.find();

        revenues.forEach((revenue) => {
              promiseMemo.push(this.revenueMgmtRepository.update(revenue));
              
              ... revenue assertion logic. And it throws somewhere in this loop.

             // Not safe. because it has two async logic inside. @Transactional doesn't wait until all the async calls are done.
              promiseMemo.push(this.secondApi(revenue));   

        })

       await Promise.all(promiseMemo);   // Not called, when an exception occured in a loop.
  }

  private async secondApi(revenue) {
    const foundAccount = await this.hostAccountMgmtService.findAccountByHost(entity.hostId);

    revenue.setAccount(foundAccount);

    await this.revenueTxMgmtRepository.insert(rtx);  // This could be called out side of the transactional scope.
  }

Conclusion,

It's not safe to use @transactional in a complex situation.

I am not sure that I can manage the transactional scope per method or per request like Spring Transaction does in any Node frameworks.

I have to accept that asynchronous calls are too complicated to be handled.

Additionally,

Promise.all is not safe to use in a transaction. The same issue happens in running transactional logic manually without cls-hook.

For loop or sequential Promise Runner could be a solution.

[odavid]

Hi @matiasraisedreal and @jchapelle - in 0.1.17, I've added Logging to the Transactional decorator.
Will be happy if you could apply it, so we can see what's going on.

During the weekend, I manage to reproduce the same behavior as you got using a test NestJS application.

After debugging it, I noticed the following reasons for this to happen:

First reason (When extending the BaseRepository)

This is the typeorm module I had...

@EntityRepository(Post)
export class PostRepository extends Repository<Post>{}
...

@Module({
  imports: [
    TypeOrmModule.forRoot({
      type: 'postgres',
      host: 'localhost',
      port: 5432,
      username: 'postgres',
      password: 'postgres',
      entities: [Post],
      synchronize: true,
      logging: "all",
      }),
      TypeOrmModule.forFeature([Post])
  ],
  exports: [],
  controllers: [AppController],
  providers: [AppService, ],
})

...
@Injectable()
export class AppService {
  constructor(
    private readonly postRepository: PostRepository,
  ) {}

  @Transactional()
  async createPost(message: string): Promise<Post>{

    console.log(`this.postRepository: ${this.postRepository.constructor.name}`)

    const p = new Post()
    p.message = message
    const ret = await this.postRepository.save(p)
    throw Error("Not save")
...
    return ret
  }

I've called console.log() inside the createPost method and it printed: Repository instead of PostRepository...

After debugging it a bit, I've noticed that I needed to expose also the PostRepository within the application module, otherwise it will not fail, but will inject a Repository instead of PostRepository and therefore the manager was not the right one within the repository causing typeorm to create a Transaction during save() with a different EntityManager

Options to solve it

• In the application module, expose also the PostRepository

  imports: [
    TypeOrmModule.forRoot({
      type: 'postgres',
     ...
   }),
// Instead of 
   TypeOrmModule.forFeature([Post])
// Change to 
   TypeOrmModule.forFeature([Post, PostRepository])

• By patching the Repository prototype

// Instead of ...
async function bootstrap() {
  initializeTransactionalContext()
  const app = await NestFactory.create(AppModule);
  await app.listen(3000);
}
bootstrap();

// Change to 
async function bootstrap() {
  initializeTransactionalContext()
  // Patch typeorm Repository, so every Repository will be able to return the right `manager`
  patchTypeORMRepositoryWithBaseRepository()
  const app = await NestFactory.create(AppModule);
  await app.listen(3000);
}
bootstrap();

Looking forward to get your answers on that matter...

Cheers

[preterer]

The issue here is this code is not resolving promises whenever it fails.

  public async mainApi() {
        const promiseMemo = [];  // Keep promises 
        const revenues = await this.revenueMgmtRepository.find();

        revenues.forEach((revenue) => {
              promiseMemo.push(this.revenueMgmtRepository.update(revenue)); // promise is started
              
              // error is thrown, all started promises are not resolved/awaited/canceled, 
             throw new Error();

              promiseMemo.push(this.secondApi(revenue));
        });
       
      // error is never caught, so function never gets here
      // promises started before the error are still running
      await Promise.all(promiseMemo);   // Not called, when an exception occured in a loop.
  }

The "easy" solution is to wrap the forEach call in a try-catch block, cancel/resolve all pending promises and then throw the caught error to handle it wherever else you are handling it right now.

You can also refactor your code so it won't start promises in a forEach loop, but rather use a map like:

await Promise.all(revenues.map(revenue => doSomething(revenue).then((result) => {
  // do your checks here
  return doSomethingElse(revenue);
}));

Then whenever the checks fail, your Promise.all call will already be made, so it will handle canceling all other pending promises. Downside is the code will wait for the first call to finish, before processing, so it will most likely be slightly slower.

Anyway I'm using Promise.all a lot, and it works perfectly fine with the library.

[odavid]

Thanks @preterer for clarification.
I think this issue became confusing and too many disconnected threads.

I will close it. If any of you guys feel it needs to be opened, please feel free to create a new one with a reference to it.