Uncaught exception in pool.query

[thorbenziemek]

Hi,

I'm seeing rare crashes on this line in one of my applications with the following error message:

Cannot read properties of undefined (reading 'once')

https://github.com/sidorares/node-mysql2/blob/489154f98224fad8036b87c63df8b736ede1c4aa/lib/pool.js#L153

It seems to happen (for example) when a query is killed from the server side (unfortunately I have not yet been able to reproduce this locally, but I see it happening on an AWS RDS MySQL instance). From what I have understood so far, the connection.addCommand method must have been replaced with _addCommandClosedState already in this state (so query(cmdQuery) will return undefined) - I don't understand why it does run into conn.query at all in this case (would expect the getConnection to return an error here, but this might just be a rare (?) race condition.

My question is: is it by intention that in the catch block, the error is just thrown instead of being propagated to the cb (or the cmdQuery)? In the execute function right below query that is the case - and it looks to me like that would work like expected here as well.

My idea would be to replace throw e; with cmdQuery.emit('error', e) but would appreciate any guidance whether that is a good idea, it looks a bit like I'm missing some difference here.

[thorbenziemek]

I don't know if this really makes it clearer, but to simulate the uncaughtException, this could be used as the query function (throws an arbitrary error - if we keep the throw, it will lead to an uncaughtException, regardless of whether the connection has an error handler registered):

...
query(sql, values, cb) {
    const cmdQuery = Connection.createQuery(
      sql,
      values,
      cb,
      this.config.connectionConfig
    );
    if (typeof cmdQuery.namedPlaceholders === 'undefined') {
      cmdQuery.namedPlaceholders = this.config.connectionConfig.namedPlaceholders;
    }
    this.getConnection((err, conn) => {
      if (err) {
        if (typeof cmdQuery.onResult === 'function') {
          cmdQuery.onResult(err);
        } else {
          cmdQuery.emit('error', err);
        }
        return;
      }
      try {
        // XXXXXX THROW ERROR for demo
        throw new Error("Simulated error.");
        conn.query(cmdQuery).once('end', () => {
          conn.release();
        });
      } catch (e) {
        conn.release();
        
        // if we keep throw e; here, this will raise an uncaughtException:
        // throw e;

        // XXXXXX SEND ERROR TO CALLBACK INSTEAD OF throw
        const actualCb = cb || values; // values is optional
        actualCb(e);
        // XXXXXX
      }
    });
    return cmdQuery;
  }
...

[thorbenziemek]

fyi, I'm not so sure whether the calling the callback with the error really is what is desired in terms of "throwing if no one listens any more" etc. Currently I use this workaround (I'm using the promise wrapper):

    async queryPool(query: string, values?: any) {
        const conn = await pool.getConnection();
        try {
            const result = await conn.query(query, values);
            return result;
        } finally {
            conn.release();
        }
    }

I can live with this workaround for now.