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

[QUESTION] Connection pool returning same connection concurrently #2325

Open
danigomez opened this issue Dec 29, 2023 · 3 comments
Open

[QUESTION] Connection pool returning same connection concurrently #2325

danigomez opened this issue Dec 29, 2023 · 3 comments

Comments

@danigomez
Copy link

danigomez commented Dec 29, 2023

Hi,
is it possible that the connection pool returns the sames connection reference if the getConnection is called concurrently?

For example, i'm using the following code to get a free connection on the pool
const connection = await mysql.pool.getConnection();

And after about 2 years of using this code, i found an issue where the call of LAST_INSERT_ID() returned the same inserted ID on two different Promises, which i think is very weird and for what i could tell, the only logical explanation for that is that the promises got the same connection and there was a race condition that returned the same value on both.

Take into account that i'm always following the pattern <get connection> => <insert data on connection> => < get last_insert_id()>=> <release connection>, that's why i think that the issue that i'm having is that the getConnection function is returning the same connection while taht connection is being used by another promise.

Does that makes sense?
Thanks

@sidorares
Copy link
Owner

sidorares commented Dec 30, 2023

is it possible that the connection pool returns the sames connection reference if the getConnection is called concurrently?

Should not be possible

And after about 2 years of using this code, i found an issue where the call of LAST_INSERT_ID() returned the same inserted ID on two different Promises
In this scenario do you always expect insertId incremented by both calls? It's possible that one query finished, then you acquired the same connection (after it was released) from another user and got same insertId.

Can you also check connection.threadId to see if these are same or different connections? If two different connections query are executed in parallel, I'm not sure hoq mysql server handles concurrency in that case

@danigomez
Copy link
Author

Thanks for you answer!

n this scenario do you always expect insertId incremented by both calls? It's possible that one query finished, then you acquired the same connection (after it was released) from another user and got same insertId.

Yes, exactly, take into account that i'm running the promises using a Promise.all, and i'm not explicitly sharing the connection but always getting a new one from the pool each time.

These are the steps that i'm running

  1. Build an array with multiple Promises
  2. On each Promise, get free connection from pool using getConnection
  3. Insert some data on auto incremented table
  4. Get LAST_INSERT_ID(), here i got the same ID on more than one promise

If i'm not mistaken, making an insert before getting last_insert_id ensures that i will get a different id after each query.
So the only case that i can think of to get the same id is that both promises were using the same connection at the same time and after both finishes the step 3 both will get the same id on step 4.

This only happened to me once after having this code running for more than two years, so it was a DB glitch or a very weird edge case of the getConnection call that returned the same connection.

Following is simplified version of the code that i'm running to make it more clear:

const connection = await mysql.pool.getConnection();

await connection.beginTransaction();

/*
Should the insertion conclude on both promises prior to invoking LAST_INSERT_ID, I'd obtain identical IDs on both, provided the same connection is shared between them.
*/
await connection.query({
  sql: `
      INSERT INTO
        testable (testdata)
      VALUES 
        (?)
    `,
  values: ['thisisateststring'],
});

const [[{ id }]] = await connection.query({
  sql: `
      SELECT LAST_INSERT_ID() AS id; 
    `,
}) as any;
    
await connection.commit();
connection.release();

I'll also take a look to the threadId property, it could help to make sure that is the same connection. Also i'll try to write a test case to reproduce this scenario

danigomez referenced this issue Jan 22, 2024
#2384)

* fix: The removeIdleTimeoutConnectionsTimer did not clean up when the pool was closed.

* test: when the pool is closed, it should clean up timers.
@cristianorvf
Copy link

cristianorvf commented Sep 9, 2024

@danigomez i confirm this "problem" even on version 3.11.0, have stumbled upon it last night and took a peek at the code. This basically a "double release" problem within mysql2 pool. The code is very small and easy to understand, take a look at pool.js and pool_connection.js.

Basically when you release a connection it does one of two things in normal situations: add itself to the freeConnections queue or immediately gets one connection from the waiting queue connectionsQueue and releases itself to it. The problem is that basically there is no check over adding the same connection to the freeConnections more than once or releasing it to the next waiting request on connectionsQueue more than once, meaning the same connection WILL be given to a different requestor should you call connection.release() in your code more than once over a "unit of work".

This wont bother most of people due to the use of some ORM or helper classes which already wraps the query execution correctly or even due to pool size VS simultaneous connection usage peak. In my case was by pure chance as im developing a data layer solution for my company architecture. I looked at the code for an hour or so.. there are solutions for this, i just dont think its worth it as is not exactly a bug.

Run the code below to see it happen (normally breaks inside the minute)

const mysql = require("mysql2/promise");
const sleep = async (ms) =>
  new Promise((res, _) => {
    setTimeout(res, ms);
  });
const { log } = console;
async function main() {
  const pool = mysql.createPool({
    host: "localhost",
    user: "xxxx",
    password: "xxxxx",
    database: "xxxxx",
    waitForConnections: true,
    connectionLimit: 1,
    queueLimit: 0,
  });

  setInterval(() => runLoop(pool, "PROCESS_A", 100), 1000);
  setInterval(() => runLoop(pool, "PROCESS_B", 250), 1000);
  await sleep(60 * 1000);
  await pool.end();
  process.exit(0);
}

async function runLoop(pool, name, ms) {
  try {
    for (let i = 0; i < 10; i++) {
      const connection = await pool.getConnection();
      log(name, "- loop=", i, "thread=", connection.threadId, "ACQUIRED");
      try {
        await connection.query("SET TRANSACTION ISOLATION LEVEL SERIALIZABLE");
        await connection.query("START TRANSACTION");
        // Some other work that delays.. pushing to queues, reading files, etc...
        await connection.query("COMMIT");
      } catch (error) {
        console.error(`Error ${i + 1}:`, error);
        process.exit(1);
      } finally {
        connection.release();
        log(name, "- loop=", i, "thread=", connection.threadId, "RELEASED");
        connection.release();
        log(name, "- loop=", i, "thread=", connection.threadId, "DOUBLE RELEASED");
      }
    }
  } catch (error) {
    console.error("Error acquiring connection:", error);
  }
}

main();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants