tediousjs / node-mssql

Microsoft SQL Server client for Node.js
https://tediousjs.github.io/node-mssql
MIT License
2.23k stars 467 forks source link

ECONNCLOSED Error with Connection Pool #1462

Closed gryphus1153 closed 5 months ago

gryphus1153 commented 1 year ago

Connection Pool gives ECONNCLOSED error after random number of days.

Expected behaviour:

Expected Pool to be self healing and recover.

Actual behaviour:

My app creates 2 seperate connection pools to connect to two different Databases on same SQL Server. This will work for a few days buit after some number of days (Unknown number of days, inconsistent) all further queries will fail and not reconnect until the app is restarted.

dbSingleton.js - Class to create connection Pool based on config paramaters

const Sql = require("mssql");

class dbSingleton {
    constructor(config) {
        this.pool = new Sql.ConnectionPool(config);
        this.poolConnect = this.pool.connect().catch((error) => console.log(error));
    }
    async procedureCall(procedure, payload) {
        await this.poolConnect;
        let request = this.pool.request();

        for (const [key, value] of Object.entries(payload)) {
            // Check if datatype is included
            if (Array.isArray(value))
            {
                request.input(key, value[0], value[1])
            }
            else
            {
                request.input(key, value);
            }
        }
        return await request.execute(procedure);
    }
    async getPool() {
        await this.poolConnect;
        return this.pool;
    }
}

module.exports = {
    dbSingleton,
};

Connector.js - Creates new dbSingleton Class with a given config.

const config = require("../config");
const { dbSingleton } = require("./dbSingleton");

const dbConnector = new dbSingleton(config.mssql_config);

module.exports = {
    dbConnector
};

Function Call - Calls a stored procedure with given key-value pairs

const Connector = require('./Connector');
async function procedureCall(procedureName, params) {
    try {
        let res = await Connector.dbConnector.procedureCall(
            procedureName,
            params
        );
        return res;
    } catch (error) {
        // handle error here
        console.log(error);
    }
}

Error Message

ConnectionError: Connection is closed.
    at Request._execute (/src/node_modules/mssql/lib/base/request.js:557:37)
    at Request._execute (/src/node_modules/mssql/lib/tedious/request.js:700:11)
    at /src/node_modules/mssql/lib/base/request.js:519:12
    at new Promise (<anonymous>)
    at Request.execute (/src/node_modules/mssql/lib/base/request.js:518:12)
    at dbSingleton.procedureCall (/src/dbConnectors/dbSingleton.js:25:30)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async procedureCall (/src/workflow.js:518:17)
    at async handler (/src/workflow.js:153:28) {
  code: 'ECONNCLOSED'

Configuration:

"options": {
      "encrypt": true,
      "trustServerCertificate": true
    },
    "requestTimeout": 300000

App is able to successfully connect to SQL Server.

Software versions

dhensby commented 1 year ago

You're using a very old version of this library, so my first advice would be to upgrade, as there are likely pooling improvements you're not getting the benefit of. For example in v7 pre-flight validation is performed on connections before releasing them from the pool.

imaginevinay commented 1 year ago

@dhensby Facing the same issue in mssql v 9.0.1

gryphus1153 commented 1 year ago

Hi, flollowing @dhensby's advice I've updated to a newer version of the library v9.1.1, but I was still having this issue, Somehow what resolved it was setting trustServerCertificate: false.

I'm running a self signed cert on the sql server but I pass the CA chain via the NODE_EXTRA_CA_CERTS command.

It is possible that my ca cert is invalid, but it is quite odd that the initial connection is allowed but is only recognized after some time.

dhensby commented 1 year ago

That sounds like a red herring - the change to validating the server certificate or not wouldn't prevent connections being closed after a prolonged period.

Looking at the error you're getting ECONNCLOSED / Connection is closed. - that comes from the pool internally if the pool isn't connected, so something is happening in your app to cause the connection pool to be closed after a prolonged period.

Your code has a slight error in that when you await your this.poolConnect promise, it is the original connection promise and not a new one if your pool has somehow closed. Also, if your connection attempt rejects, the error is swallowed, logged and that rejected promise is cached, and your application will never try to connect again, instead always returning the rejected promise.

It may be better to simply await a new connect() call every time:

const Sql = require("mssql");

class dbSingleton {
    constructor(config) {
        this.pool = new Sql.ConnectionPool(config);
-        this.poolConnect = this.pool.connect().catch((error) => console.log(error));
+        this.connect();
    }
    async procedureCall(procedure, payload) {
-        await this.poolConnect;
+        await this.connect();
        let request = this.pool.request();

        for (const [key, value] of Object.entries(payload)) {
            // Check if datatype is included
            if (Array.isArray(value))
            {
                request.input(key, value[0], value[1])
            }
            else
            {
                request.input(key, value);
            }
        }
        return await request.execute(procedure);
    }
    async getPool() {
-        await this.poolConnect;
+        await this.connect();
        return this.pool;
    }
+    async connect() {
+        return this.pool.connect().catch((error) => console.log(error));
+    }
}

module.exports = {
    dbSingleton,
};

edit: It's worth noting that we're still swallowing any potential connection errors here, so you are still able to run into the exact same error, just this will be more likely to recover after repeated procedure calls. You'd have to implement some kind of connection retry logic in the connect method if you want to be even more resilient to connection problems.