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

Fix return duplicate transaction promise for standalone transactions #3328

Merged
merged 1 commit into from Jul 4, 2019

Conversation

Projects
None yet
2 participants
@vonagam
Copy link
Contributor

commented Jul 3, 2019

So it seems like #3319 was a mistake and having a duplicate is needed because original execution promise does have catch attached so it will never produce unhandled rejections. So if you do not attach catch to executionPromise nobody will tell you that your transaction queries are failing (except this debug statement).

Solution: promise duplication but only for standalone transactions.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Jul 3, 2019

@vonagam One test is failing.

@vonagam vonagam force-pushed the vonagam:fix-transaction-3 branch from 0efc89d to d34a072 Jul 3, 2019

@vonagam

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

Should be fixed now. But executionPromise will behave differently for standalone and callback style transactions: for standalone you must catch rejections from executionPromise, while for callback style it is optional (since they have access to _promise itself which behaves the same).

@vonagam vonagam force-pushed the vonagam:fix-transaction-3 branch 2 times, most recently from a0e2bb0 to b0becd1 Jul 4, 2019

@vonagam

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

Fixed typo in transactionProvider.

Also transactionProvider returns a standalone transaction retriever and so executionPromise should be handled in tests where a provider is used otherwise they might produce unhandled rejections which are actually tests failures.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

commented Jul 4, 2019

@vonagam A different test on postgreSQL is failing now.

@vonagam

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

Yes, i know. This test was failing before, but silently, it was producing unhandled rejections before, now the test properly fails because executionPromise is listened to.

Here is the link to an unhandled rejection warning for this test when it was introduced, so it was actually not passing from the start. (By the way i include line numbers in my links to travis logs, but unfortunately travis does not scroll to specified rows)

I can't run postgres tests (some issue with a connection... maybe figure it out later), i was running only sqlite tests on my machine before.

@vonagam

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

Okay, managed to run this test. The issue is on this line. For me postgres res object does not have context property (it does have direct sql property equal to "ROLLBACK" though). So it fails the check and rejects with undefined.

I think this should be fixed together with first issue from #3309.

@vonagam vonagam force-pushed the vonagam:fix-transaction-3 branch from b0becd1 to 4a130e1 Jul 4, 2019

@vonagam vonagam force-pushed the vonagam:fix-transaction-3 branch from 4a130e1 to 2ded6a8 Jul 4, 2019

@vonagam

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

Fixed it.

get(res, 'context.sql', '').toUpperCase() === 'ROLLBACK' &&
this.doNotRejectOnRollback
) {
if (this.doNotRejectOnRollback && /^ROLLBACK\b/i.test(sql)) {

This comment has been minimized.

Copy link
@kibertoad

kibertoad Jul 4, 2019

Collaborator

Should this be case-sensitive?

This comment has been minimized.

Copy link
@kibertoad

kibertoad Jul 4, 2019

Collaborator

This change is probably also needed for db dialect transaction implementations that have their own rollback handling.

This comment has been minimized.

Copy link
@kibertoad

kibertoad Jul 4, 2019

Collaborator

I mean https://github.com/tgriesser/knex/blob/master/src/dialects/mysql/transaction.js, doesn't look like any other dialect has this logic.

This comment has been minimized.

Copy link
@vonagam

vonagam Jul 4, 2019

Author Contributor
  • Current implementation is case-insensitive, so that's why i added /i flag to make the regexp case-insensitive too. But actually it does not need to be case-insensitive, since only valid way to rollback or commit is though trx.rollback() or trx.commit(), only they set status argument, so only they _resolve() or _reject(), so only they release a connection properly, and they use upper case, so actually there is no need for /i flag, i just kept it to be similar to current implementation.

  • Other dialects do usual rollback fine, since only postgres was failing the test. But i assume they do nested rollback badly (first issue). So yeah, other dialects should be updated to treat rollbackTo() queries the same way as rollback(), but it will require new tests for such cases and new PR. I just wanted to make current tests pass.

This comment has been minimized.

Copy link
@kibertoad

kibertoad Jul 4, 2019

Collaborator

Fair enough!

@kibertoad kibertoad merged commit a0e9be5 into tgriesser:master Jul 4, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on fix-transaction-3 at 88.103%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.