mariadb-corporation / mariadb-connector-nodejs

MariaDB Connector/Node.js is used to connect applications developed on Node.js to MariaDB and MySQL databases. MariaDB Connector/Node.js is LGPL licensed.
GNU Lesser General Public License v2.1
366 stars 91 forks source link

Return results awaiting pool connection.release() #211

Closed H14 closed 1 year ago

H14 commented 1 year ago

From the documentation, I understand that pool.getConnection() resolves with a connection. Connection must be given back to pool using theconnection.release()method.

The code example uses a console.log statement to show when the results are available.

const pool = mariadb.createPool({ 
    host: 'mydb.com', 
    user:'myUser' 
});

let conn;
try {
    conn = await pool.getConnection();

    console.log("connected ! connection id is " + conn.threadId);

    await conn.release(); //release to pool
} catch (err) {
    console.log("not connected due to error: " + err);
}

However, if I want to actually return them from within a function, this can only be done after awaiting the pool release.

async function results() {
  try {
    const conn = await pool.getConnection();

    const rows = await connection.execute('SELECT * FROM Table');

    await conn.release(); //release to pool

    return rows;
  } catch (err) {
    throw("not connected due to error: " + err);
  }
}

Is this the right way to deal with pool connections? Will awaiting the release have any performance impact?

Thanks

Orel-A commented 1 year ago

No, I think there is something wrong in the code. The connection variable should be out of the scope of the try-catch, and be initialized to null. Then you could use: finally { if (conn) await conn.release() } the getConnection function could also throw an error. in the catch you should set the error to a variable out of the scope so that after the try-catch-finally you will check for its instance with if (err instanceof SqlError) and then relevant err.code

H14 commented 1 year ago

Okay, I think I understand the finally bit now.


async function results() {
    let connection;

    try {
        connection = await pool.getConnection();

        const result = await connection.query(`SELECT NOW()`);

        return result;
    } catch (err) {
        throw err;
    } finally {
        if (connection) {
            await connection.release();
        }
    }
}

Could you perhaps elaborate a bit more on the error handling? Why should I check what instance the error is of? Why should that happen outside the try/catch/finally? I know just rethrowing a catched error is unnecesary.

Thanks!

Orel-A commented 1 year ago

You can rethrow a catched error if you can't handle it. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/try...catch#conditional_catch-blocks you can use a custom error handler like this:

app.use((err, req, res, next) => {
  if (err instanceof SqlError) {
    console.error(err.stack);
    return res.status(500).render('error', {code: 'sqlerror'});// beautiful 500 error page
  }
  next(err);
});

an example:

let conn = await db.getConnection();
try {
  await conn.beginTransaction();

  // some calls to the database here

  await conn.commit();
}
catch (err) {
  await conn.rollback();
  if (err instanceof SqlError && err.code === 'ER_DUP_ENTRY' && err.text.endsWith("'surveyid'")) return res.status(500).render('error', {code: 'surveyid_dup'});
  else throw err;
}
finally {
  await conn.release();
}

the call to getConnection is outside because expressjs 5.0 will catch it. if you are using 4.0 then you will need to call next(err)

rusher commented 1 year ago

technically it's not mandatory to wait for conn.release(). this function will never throw an exception. If there were some, connection will just be discarded from pool transparently.

conn.release() will rollback any transaction if there is some pending unless explicitly set by option (noControlAfterUse if set will avoid executing a rollback command)

so you can use :

conn = await pool.getConnection();
try {
  await conn.beginTransaction();
  // some calls to the database here
  await conn.commit();
} finally {
  conn.release();
}
rusher commented 1 year ago

documentation added :

connection.release() → Promise

When connection comes from pool only connection.release() is an async method returning an empty promise success. This function will never throw an error. default behavior is that if there is a transaction still open, a rollback command will be issued, and connection will be release to pool.

2 options might interfere:

const conn = await pool.getConnection();
try {
  await conn.beginTransaction();
  await conn.query("INSERT INTO testTransaction values ('test')");
  await conn.query("INSERT INTO testTransaction values ('test2')");
  await conn.commit();

} finally {
  conn.release();
}