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

Transactions: Is the current implementation bulletproof? #35

Open
SebastianApel opened this issue May 24, 2019 · 4 comments
Open

Transactions: Is the current implementation bulletproof? #35

SebastianApel opened this issue May 24, 2019 · 4 comments
Labels
help wanted Extra attention is needed question Further information is requested

Comments

@SebastianApel
Copy link

Hi Jeremy,
I was thinking about the transaction implementation. My test scenario now works fine (with the workaround, see issue #34) in the "single client test case" scenario.

But I am not sure what would happen under load with a lot of parallel transactions. What happens when a parallel execution also uses the connection - how can you be 100% sure that the "other" execution is not issuing SQL statements over the connection DURING the transaction (i.e. after the START TRANSACTION, but before the COMMIT / ROLLBACK)?

For the COMMIT case, that would not be critical, but for the ROLLBACK case, it would result in data loss (because it would appear to the "other" thread that the statement executed just fine - but later it gets rolled back, so the data will not be in the database... Ouch).

Have you thought about that case? Are you sure your current approach is bulletproof?

An idea for an improved approach could be:

  • get a NEW, PRIVATE connection (i.e. one that is not shared)
  • use only that for the transaction
  • quit the transaction after the COMMIT / ROLLBACK

Happy to discuss...

@jeremydaly
Copy link
Owner

Hi @SebastianApel,

Thanks for reporting this. If you are using AWS Lambda, there should only be one connection per invocation (i.e. concurrent user), so you should never get the race condition you're referring to. I'm not sure how this would work across other providers exactly, so input from others would be helpful.

Let me look at #34 in more detail.

  • Jeremy

@jeremydaly jeremydaly added help wanted Extra attention is needed question Further information is requested labels May 24, 2019
@SebastianApel
Copy link
Author

SebastianApel commented May 26, 2019

Hi @jeremydaly,
thanks for your reply. I am not familiar enough with Lambda to jugde that case.

I am currently using the library in an non-Lambda usecase (because it promissifies mysqljs nicely, it handles connection timeouts "well" out of the box, and I liked the transaction idea). My current setup is based on express, so my rationale in that case is valid - right? (I'm pretty new to node, so I might be wrong here - feedback is highly welcome).

If my rationale holds, then at least a warning in the README would be very helpful for the users.

Sebastian

@bluedusk
Copy link

I think this lib is for lambda based mysql connection management, if you don't use lambda, I don't see necessity to use this.

@douglasgsouza
Copy link

I have also been thinking about this issue, especially regarding the reuse of the connection.

The way that the transaction () method was implemented seems to be safe, as it would execute all queries at the same time, calling the rollback in case of error. Sufficient for most use cases.

But I will post here my reflection on this issue:

It can be a problem when trying to use it procedurally. In the example below I have other complex non-sql operations that depend on executing queries in stages, to check for unique constraints.

Example:

const transaction = mysql.transaction ();
try {
  transaction.query ("INSERT INTO table (name) VALUES (?);
  --- do a non-sql operation only if the above query is successful.
  transaction.query ("INSERT INTO table (name) VALUES (?);
  await transaction.commit (); // transaction is only initiated here and querys are executed only here.
} catch (e) {
  await transaction.rollback ();
}

In this case, queries are only executed on commit and if the first query causes an error, the non-sql operation could not be performed.

So I thought of a second approach, starting the transaction manually with individual queries (START TRANSACTION). But then I don't know if I would have a problem because I was reusing connections with simultaneous lambda executions.

My suggestion is:

  • create a separate beginTransaction () method that returns a transaction instance that uses a different connection exclusively for each transaction.
  • maintain an array with these transaction connections, which can be released through the manager.

I would even like to help with the implementation, but I don't even know where to start.

Thanks Jeremy, this package and your blog articles are fantastic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants