Fix operations on ended transactions #230

zbarbuto · 2017-03-20T11:49:26Z

Description

Issue

This fixes a pretty critical issue where operations can occur on non-transactional connections when the user expects them to land in a transaction. Tests added which fail on master but pass here.

Example use case:

Begin a transaction.
Perform some creates on the transaction
An external event (e.g. timeout) triggers a rollback
Additional creates are allowed to continue despite the transaction no longer being active
These creates still hit the database when the programmer expects them not to as the transaction has rolled back (finished)

Solution:

I've added a uuid to each transaction which exists on the transaction itself and is tagged onto the current connection. Whenever the transaction is committed or rolled back ('finished') the id is removed from the connection but remains on the transaction. Any attempts to execute sql on the transaction checks the transaction id against the specified connection and will fail if the ids do not match.

Related issues

Transaction timeout not preventing DB calls #129

Checklist

New tests added or existing tests modified to cover all changes
Code conforms with the style
guide

slnode · 2017-03-20T11:49:27Z

Can one of the admins verify this patch?

slnode · 2017-03-20T11:49:27Z

Can one of the admins verify this patch?

slnode · 2017-03-20T11:49:27Z

Can one of the admins verify this patch?

zbarbuto · 2017-03-20T11:50:31Z

@superkhau @raymondfeng @jannyHou

zbarbuto · 2017-03-20T11:57:40Z

I suppose the id need not be a uuid. Could easily be an incrementing integer. Open to opinions.

zbarbuto · 2017-03-20T22:00:56Z

Not sure what was happening with the failed tests on node 4.x and 6.x - all passing here on my end 7.x but have fixed the eslint errors in the test files.

zbarbuto · 2017-03-22T21:24:07Z

@jannyHou Any idea what might be causing the failures? Is it something I can fix up?

zbarbuto · 2017-03-22T21:33:11Z

Seems to be unrelated to this PR:

transactions bulk.should work with bulk transactions
sorry, too many clients already

What is the connection limit on the build server's postgres?

jannyHou · 2017-03-23T15:11:25Z

lib/transaction.js

+ var tx = new Transaction(connector, connection);
+ tx.txId = uuid.v1();
+ connection.txId = tx.txId;
+ cb(null, tx);


I think here we should callback connection instead of tx?

The goal here is to make the new connection and transaction and say "this connection is currently working on this transaction" allowing for the error if you pass in a transaction with a connection id that doesn't match.

If you callback with connection, loop back connector will make a transaction out of it and pas it up the chain. This means you how no tie between the id of the connection and the current transaction.

jannyHou · 2017-03-23T15:14:53Z

@slnode test please

jannyHou · 2017-03-23T16:17:03Z

@zbarbuto In my opinion the expected behaviour is:
If we do tx.commit(), then use that expired tx in other operations like create, postgresql db should return us an error.
NOT
We add a uuid to check the transaction still active on our end

So if the example use case you described in the issue happens, it looks like a bug of nodejs postgresql driver to me :(

@raymondfeng any thought?

zbarbuto · 2017-03-23T19:58:30Z

I don't think it's a problem with the driver since the driver doesn't actually handle transactions natively. You're just executing normal transaction sql on a single connection:

https://github.com/brianc/node-postgres/wiki/Transactions

This assumes you're managing the connection in such a way that no other operations can execute on it until the transaction has resolved. Their example does this by not passing the connection outside the callback chain.

In the case of the loop back connector, the connection gets released from the transaction but nothing notifies an ongoing promise chain that the connection being used is no longer part of a transaction. If it doesn't error out creates, they just end up in the database which is very dangerous and can result in orphaned records:

ModelA.create(tx) .then(() => ModelB.create(tx)) // Tx times out here .then(() => JoinTable.create({ model a, model b, tx})

If the transaction rolls back, a and b don't his the database (they were part of the transaction) but the join between them does (it was on a connection that no longer has an active transaction). Surely this can't be expected behaviour.

jannyHou

LGTM looking into the CI failure reason

jannyHou · 2017-03-24T21:01:43Z

@slnode test please

jannyHou · 2017-03-24T21:26:02Z

@zbarbuto I think the default poolSize is 10, see https://github.com/brianc/node-postgres/blob/master/lib/defaults.js#L40, while increasing the limit doesn't solve the issue but make it worse...

And my commit is testing the limit increase, sorry if that confuses you, feel free to remove it if you don't need.

zbarbuto · 2017-03-24T22:20:47Z

Thanks for trying. The test that is failing is one that I added a little while ago so strange that it starts failing now.

Just waiting on approval from @raymondfeng?

Did you want me to switch from uuid to an incrementing value to avoid unnecessary dependencies?

raymondfeng · 2017-03-27T20:30:19Z

lib/postgresql.js

- transaction.connector === this) {
+ if (transaction && transaction.connector === this) {
+ if (!transaction.connection) {
+ return callback(new Error(g.f('Connection does not exist')));


Please wrap it into process.nextTick or setImmediate.

raymondfeng · 2017-03-27T20:30:26Z

lib/postgresql.js

+ return callback(new Error(g.f('Connection does not exist')));
+ }
+ if (transaction.txId !== transaction.connection.txId) {
+ return callback(new Error(g.f('Transaction is not active')));


Please wrap it into process.nextTick or setImmediate.

zbarbuto · 2017-03-29T22:19:42Z

No further ideas on why the tests might be failing?

jannyHou · 2017-03-30T15:44:18Z

@zbarbuto I am making time to debug why done() is called multiple times, and why it complains too many connections....

jannyHou · 2017-04-06T17:16:05Z

The same failure also happens on master branch, it fails on only one machine and I set up a windows vm using same environment, but could not reproduce the error.

I will create another PR just debug why it fails. Sorry for the delay of checking this pr in, it looks good but we have to fix the test :)

zbarbuto · 2017-04-06T20:11:18Z

No problems. Thanks for your help @jannyHou

dhr00v · 2018-04-23T10:17:27Z

@zbarbuto I have one question.

https://github.com/zbarbuto/loopback-connector-postgresql/blob/1361460cea86fadd14a68aa8fe9c55f14ecb9a6d/lib/transaction.js#L58-L86

What if 'ROLLBACK' query is executed, but the callback hasn't been called. So basically releaseConnection hasn't been called and connection.txId is not null. So at this time, any other query in the same transaction can still go through, right? Shouldn't we move connection.txId in rollback function itself?

zbarbuto · 2018-04-23T22:34:35Z

@dhr00v Wh would the callback not be executed if ROLLBACK was? Could you put together an example repo for an instance where this might be the case?

sahilarora946 · 2018-04-24T05:57:04Z

@zbarbuto After the ROLLBACK was executed, callback will be sent to the event loop to be executed. Event loop can pick any of the available callbacks to execute. It can pick another function that can do database calls in the same transaction and because releaseConnection has not been called, the statement will execute.

zbarbuto · 2018-04-24T06:04:26Z

Hmm - I see what you're saying. Do you have a proposed solution for this? Would you be willing to submit a PR?

sahilarora946 · 2018-04-24T06:11:34Z

Ok. I will submit a PR for this @zbarbuto

sahilarora946 · 2018-04-24T07:24:16Z

@zbarbuto kindly refer to #330

dhmlau · 2018-05-25T22:07:44Z

Fixed in #330, which is released as loopback-connector-postgresql@3.3.2.

superkhau added the review label Mar 20, 2017

siddhipai added review and removed review labels Mar 20, 2017

jannyHou self-assigned this Mar 21, 2017

jannyHou suggested changes Mar 23, 2017

View reviewed changes

jannyHou requested a review from raymondfeng March 23, 2017 16:17

jannyHou approved these changes Mar 23, 2017

View reviewed changes

jannyHou force-pushed the create-on-ended-tx branch from 2fd53d9 to 0bf6311 Compare March 24, 2017 21:14

siddhipai added review and removed review labels Mar 27, 2017

raymondfeng reviewed Mar 27, 2017

View reviewed changes

zbarbuto mentioned this pull request Mar 28, 2017

Allow non-id serial properties #198

Merged

2 tasks

jannyHou force-pushed the create-on-ended-tx branch from 385c9c8 to 2876a95 Compare March 30, 2017 18:54

jannyHou mentioned this pull request Apr 9, 2017

Debug/transaction test #238

Closed

ssh24 force-pushed the create-on-ended-tx branch from b139e70 to 224240b Compare April 10, 2017 21:53

zbarbuto and others added 7 commits April 13, 2017 17:21

Fix operations on ended transactions

1cd5348

Fix eslint errors in test

c0ca091

Increase poolSize

6ebdf94

Wrap transaction error callbacks in nextTick

9de4d92

Remove poolSize

adc7d2e

Return done(err)

e744cdd

Add missing semicolons

1361460

jannyHou force-pushed the create-on-ended-tx branch from 224240b to 1361460 Compare April 13, 2017 21:22

jannyHou merged commit 039b206 into loopbackio:master Apr 13, 2017

jannyHou removed the review label Apr 13, 2017

jannyHou modified the milestones: Sprint 33, Sprint 33 - Apex Apr 13, 2017

jannyHou added the apex label Apr 13, 2017

bajtos mentioned this pull request Jun 26, 2017

Transactions are broken on v3.0.1 #258

Closed

zbarbuto mentioned this pull request Jun 27, 2017

Fix tranaction operations when called from the model #268

Merged

2 tasks

sahilarora946 mentioned this pull request Apr 24, 2018

Stop executing query if transaction has ended #329

Closed

sahilarora946 mentioned this pull request Apr 24, 2018

stop execution if rollback is called #330

Merged

jannyHou mentioned this pull request Jan 29, 2020

commit or rollback a transaction does not delete the connection's reference #411

Closed

Fix operations on ended transactions #230

Fix operations on ended transactions #230

Uh oh!

Conversation

zbarbuto commented Mar 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Checklist

slnode commented Mar 20, 2017

slnode commented Mar 20, 2017

slnode commented Mar 20, 2017

zbarbuto commented Mar 20, 2017

zbarbuto commented Mar 20, 2017

zbarbuto commented Mar 20, 2017

zbarbuto commented Mar 22, 2017

zbarbuto commented Mar 22, 2017

jannyHou Mar 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

zbarbuto Mar 23, 2017

Choose a reason for hiding this comment

jannyHou commented Mar 23, 2017

jannyHou commented Mar 23, 2017

zbarbuto commented Mar 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

jannyHou left a comment

Choose a reason for hiding this comment

jannyHou commented Mar 24, 2017

jannyHou commented Mar 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

zbarbuto commented Mar 24, 2017

raymondfeng Mar 27, 2017

Choose a reason for hiding this comment

zbarbuto Mar 27, 2017

Choose a reason for hiding this comment

raymondfeng Mar 27, 2017

Choose a reason for hiding this comment

zbarbuto Mar 27, 2017

Choose a reason for hiding this comment

zbarbuto commented Mar 29, 2017

jannyHou commented Mar 30, 2017

jannyHou commented Apr 6, 2017

zbarbuto commented Apr 6, 2017

dhr00v commented Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

zbarbuto commented Apr 23, 2018

sahilarora946 commented Apr 24, 2018

zbarbuto commented Apr 24, 2018

sahilarora946 commented Apr 24, 2018

sahilarora946 commented Apr 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

dhmlau commented May 25, 2018

Labels

10 participants

zbarbuto commented Mar 20, 2017 •

edited

Loading

jannyHou Mar 23, 2017 •

edited

Loading

zbarbuto commented Mar 23, 2017 •

edited

Loading

jannyHou commented Mar 24, 2017 •

edited

Loading

dhr00v commented Apr 23, 2018 •

edited

Loading

sahilarora946 commented Apr 24, 2018 •

edited

Loading