Idle connections not being closed by the pool

[stefanobaldo]

Based on the docs quick example, I am trying to run a simple query as follows:

const sql = require('mssql')

(async () => {
    try {
        const pool = await sql.connect('mssql://username:password@localhost/database')
        const result = await sql.query`select * from mytable where id = ${value}`
        console.dir(result)
    } catch (err) {
        // ... error checks
    }
})();

After logging the results, the process remains stucked until I press CTRL+C.

The problem is happening in the await sql.query part - it is returning a promise that seems to resolve, so I cannot figure out what is happening and why the process do not finish and keeps waiting for something.

Node 7.9.0
node-mssql 4.0.2

[patriksimek]

Are you checking for an exception presence inside catch block?

[stefanobaldo]

Yes, it does not get into the catch block

[patriksimek]

Ok, I have tested it on the same versions and it works. Could you please try to use non-async-await syntax? Just to confirm it has nothing to do with connection problems.

[stefanobaldo]

The results from the db are being displayed to stdout, so I think it has nothing with the connection, but I will try anyway.

[patriksimek]

Ahh, sorry, I didn't understand. Now its clear where the problem is - the pool doesn't close idle connections. With v4 I have updated underlying node-pool dependency to latest version and didn't noticed that the behavior has changed and it is not longer releasing idle connections by default. I will fix that.

[stefanobaldo]

That is ok. I will change the issue title since it has nothing with the async/await syntax. Thank you!

[patriksimek]

Fixed in 4.0.4. Anyway, the fix doesn't close the process when all connections are closed. You need to explicitly call pool.close().

[CnApTaK]

nothing is fixed...
pool.max does not working at all

var pool = new sql.ConnectionPool(config);

pool.on('error', function(err) {
	log.error(err);
});
pool.connect(function(err) {
	if (err)
		log.error(err);
});

setInterval(function() {
	//var request = pool.request();
	var request = new sql.Request(pool);
	var query = 'select getdate() now';
	request.query(query, function(err, res) {
		if (err)
			log.error(err);
		else
			log.log(res)
	});
}, 1000);

this example reporoduce the problem.. this mean there is no pool at all

[vangorra]

I'm using 4.0.4 and can confirm calling connectionPool.close() closes the connections.. eventually. In my case, we're using AWS lambda functions to pull data from the DB. So waiting for the connection pool to terminate is a no go. I worked around the issue my modifying the pool configuration after instantiating the ConnectionPool. This ensures our lambda function will execute quickly and will not hang around after data was returned.

Note: If you are using the non-async-await syntax, instantiating ConnectionPool will also trigger connect. So you'll have to provide a config object at instantiation time.

My Implementation:

export class DatabaseService {
  readonly connection: ConnectionPool;

  constructor(dbUri: string) {
    this.connection = new ConnectionPool(dbUri);

    this.connection['config'] = Object.assign(
      this.connection['config'] || {},
      {
        pool: {
          max: 1,
          min: 0,
          evictionRunIntervalMillis: 500,
          idleTimeoutMillis: 500,
          testOnBorrow: true,
        },
      },
    );

  async connect(): Promise<ConnectionPool> {
    // not yet connected, start the connection.
    if (!this.connection.connected && !this.connection.connecting) {
      await this.connection.connect();
    }

    return this.connection;
  }

  async disconnect(): Promise<boolean> {
    try {
      await this.connection.close();
      return true;

    } catch (e) {
      console.error('Error while disconnecting from database:', e);
      return false;
    }
  }

  ...
}
```

[CnApTaK]

I dont need to close connections. I need to work with tem like with pool. Pool have to manage them by itself, but he doesnt. Dont limit them, dont wait until some of them gets released to use it with next query, dont close them by timeout when they gets released

[josalmi]

I'm having similar issues on 4.0.4.

I have an mssql server running in docker:

docker run --rm -e ACCEPT_EULA=y -e SA_PASSWORD=Password123 -p 41433:1433 microsoft/mssql-server-linux
docker exec {container id} /opt/mssql-tools/bin/sqlcmd -U sa -P Password123 -Q 'CREATE TABLE a(name varchar(10));'

Here is the smallest script that I was able to reproduce the issue with:

const mssql = require('mssql');
const pool = new mssql.ConnectionPool('mssql://sa:Password123@localhost:41433/master')
pool.connect()
  .then(() => {
    return pool.query`SELECT * FROM a`;
  })
  .then(() => {
    return Promise.all([
      pool.query`INSERT INTO a VALUES ('b')`,
      pool.query`INSERT INTO a VALUES ('c')`
    ]);
  })
  .then(() => {
    return pool.close();
  })
  .then(() => {
    console.log('App finished');
  });

I also appended some logging as follows:

base.js

  _close (callback) {
    this._connecting = this._connected = false

    if (!this.pool) return setImmediate(callback, null)

    const pool = this.pool
    console.log('Draining pool'); // Debug
    this.pool.drain().then(() => {
      console.log('Drained pool'); // Debug
      pool.clear()
      callback(null)
    })

    this.pool = null
  }

tedious.js

_poolDestroy (tedious) {
    console.log('Closing connection'); // Debug
    return new base.Promise((resolve, reject) => {
      tedious.once('end', () => {
        console.log('Closed connection') // Debug
        resolve()
      })

      tedious.close()
    })
  }

I get this output when running the script:

Closing connection
Closed connection
Draining pool
Drained pool
App finished
Closing connection
Closed connection

The issue might be even easier to replicate if you append additional queries to Promise.all block. Below is output when 10 insert operations were scheduled concurrently.

Closing connection
Closed connection
Draining pool
Drained pool
App finished
Closing connection
Closed connection
Closing connection
Closed connection
Closing connection
Closed connection
Closing connection
Closed connection
Closing connection
Closed connection
Closing connection
Closed connection
Closing connection
Closed connection
Closing connection
Closed connection
Closing connection
Closed connection

It seems that some connections are being closed too late. I also attached wtfnode library to my script and got the following output at the end of the script:

[WTF Node?] open handles:
- File descriptors: (note: stdio always exists)
  - fd 1 (tty) (stdio)
  - fd 2 (tty) (stdio)
- Sockets:
  - 127.0.0.1:65419 -> 127.0.0.1:41433
  - 127.0.0.1:65420 -> 127.0.0.1:41433
- Timers:
  - (15000 ~ 15 s) bound connectTimeout @ myproject/node_modules/tedious/lib/connection.js:619

However, if I wait 15000ms before calling close after the requests, the script runs and stops successfully.

})
.then(() => {
  return new Promise((resolve) => {
    setTimeout(resolve, 15000);
  });
})
.then(() => {
  return pool.close();

Closing connection
Closed connection
Closing connection
Closed connection
Closing connection
Closed connection
Draining pool
Drained pool
App finished

[sleepycat]

I'm still seeing this exact problem in 4.1.0. The connection pool causes my test to hang after passing and Jest never exits. Adding pool.close() after the sql.query solves the hang issue.

describe('mssql', () => {
  it('works without confusing Jest', async () => {
    const pool = await sql.connect(config)
    const result = await sql.query`select * from new_evaluationBase;`
    pool.close()
    expect(result.recordset.length).toEqual(3)
  })
})

[Danielus]

I have the same problem with the 4.10 version. I am using the connection pool when triggering a good amount of async queries. And after I return the results from the db, I explicitly close the connection pool but still the connections used remain active and the MSSQL server shows all the unclosed connection. For a heavy use, this makes the Db server to reach the max available connections and the Node.JS server can not connect anymore .

Is there any fix I can use?

[willmorgan]

What about using a lower idleTimeoutMillis?

https://github.com/tediousjs/node-mssql#general-same-for-all-drivers

pool.idleTimeoutMillis - The Number of milliseconds before closing an unused connection (default: 30000).

[mottykohn]

Does not help to lower idleTimeoutMillis

…

On Thu, May 10, 2018 at 5:02 PM, Will Morgan ***@***.***> wrote: What about using a lower idleTimeoutMillis? https://github.com/tediousjs/node-mssql#general-same-for-all-drivers pool.idleTimeoutMillis - The Number of milliseconds before closing an unused connection (default: 30000). — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#457 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ARIl6Hsq2VGCfPL_m31qGXzSq0g3kcHVks5txKrmgaJpZM4NGtLQ> .

-- Mordechai Kohn

[aflansburg]

Not to rehash this, but still having to call pool.close() to end process. Will look into why once I have a chance - hate to present a problem (again) without a solution!

[Danielus]

I managed to fix my problems with the pool and not properly closing the connections, using the Bluebird Promises disposer. Here is a code snippet:

'use strict';

const Promise = require('bluebird');
const sql = Promise.promisifyAll(require("mssql"));


export function getConnection(mssqlConfig) {
	let connection;

	return new Promise(function(resolve, reject) {
		connection = new sql.ConnectionPool(mssqlConfig, function(err){
			if(err) {
				connection = null;
				return reject(err);
			}
			//console.log('--> Acquired MSSQL connection from the pool.');
			resolve(connection);
		});
	}).disposer(function(){
		if (connection) {
			//console.log('--> Closing MSSQL connection from the pool...');
			connection.close();
		}
	});
}

export function getPromisifiedSql(){
	return sql;
} 

And then use it:

return Promise.using(getConnection(mssqlConfig), function(connection) {
      ...
       let promisifiedSql = getPromisifiedSql(),
        request = new promisifiedSql.Request(connection);

      return request.query(completeQuery)
        .then(result => {
          ....
        }).catch(err => {
          console.log(err);
        });
});

Is basically creating a connectionPool and closing it at the end of the usage.

With other implementations I tried before, I had loads of unclosed connections in the server and on the long run it made the server to crash. In this way you can make sure that you close all the connections.
It might not be ideal (considering that if you have loads of async requests you lose some time with the pool creation each time ).

Is not the most viable solution, but is a workaround.

[willmorgan]

If you must create a connection pool for every connection request then surely you want to set max: 1, min: 1 on the pool config?

Or the better option, which is not pooling at all.

[Danielus]

Yes, indeed, the config is set to max:1, min:1

[willmorgan]

More recent versions of node-mssql have improved lifecycle management which I think should resolve this issue, so I'm closing it. Please upgrade to v4.2.2 (most recent at time of writing). If this doesn't improve things then please feel free to reopen.

Particularly this could have been fixed by #683.

[vlapo]

Facing this issue for a while in our mocha tests using mssql v4.3.0. wtfnode shows open connections to sqlserver:

[WTF Node?] open handles:
- File descriptors: (note: stdio always exists)
  - fd 1 (tty) (stdio)
  - fd 2 (tty) (stdio)
- Sockets:
  - 127.0.0.1:54139 -> 127.0.0.1:1433

But ConnectionPool is closed after call await pool.close():

ConnectionPool {
  _events: { error: [Function] },
  _eventsCount: 1,
  _maxListeners: undefined,
  config:
   { connectionTimeout: undefined,
     requestTimeout: undefined,
     stream: false,
     pool: undefined,
     options: { useUTC: false },
     server: 'localhost',
     user: 'sa',
     password: 'Admin12345',
     database: 'tempdb',
     port: 1433,
     domain: undefined,
     parseJSON: false },
  _connected: false,
  _connecting: false,
  pool: null }

This happens in irregular base but it has impact to our tests.
Debug logs:

pool(1): connection #3 created
connection(3): establishing
pool(1): connection #4 created
connection(4): establishing
connection(2): borrowed to request #115
request(115): query SELECT ...
connection(2): released
request(115): completed
connection(2): borrowed to request #116
request(116): query SELECT ...
connection(2): released
request(116): completed
connection(2): borrowed to request #117
request(117): query SELECT ...
connection(2): released
request(117): completed
    ✓ should be able to retrieve the whole tree (313ms)
====> before close
====> after close
connection(3): established
connection(4): established
connection(2): destroying
connection(2): destroyed
connection(3): destroying
connection(3): destroyed

[dhensby]

Yeah, I still have pain with tests on mocha - I just ensure I close every connection that gets created and I do that by keeping track of connections. It is a bad developer experience, though...

@willmorgan any ideas?

[vlapo]

After digging deeper I think this issue is related to node-pool library issue coopernurse/node-pool#159 (comment) or maybe some derivation of this issue.

[dhensby]

@vlapo is there a workaround we can apply to this library to mitigate the above issue?

[vlapo]

I dont think so. If my deduction is right it is internal problem of library.

[johncrisostomo]

I am also experiencing this issue. Quite surprised it's still open, kinda defeats the purpose of using a connection pool.

[willmorgan]

@dhensby You were looking into this the other day, remind me what you found? Was it Tedious, the pool, or this library?

[dhensby]

OK - so with a bit better understanding of the issue as raised here, I'm not sure there's a bug(??)

From what I understand people want the connection pool to either (1) close itself after a request is complete OR they want (2) the connection pool to automagically close itself when the process is complete.

1. This completely defeats the point of a connection pool, don't use a pool if you want this, use a single connection
2. This seems slightly impossible because how on earth is the pool meant to know when you don't want it any more? If you don't want it, close it.

---

I was experiencing the problem with mocha test suites not exiting because processes were still running (connections were open), unfortunately the only way to resolve this was to follow the mocha docs and properly manage your connections (ie: closing them in the after functions).

If the mocha process is still alive after your tests seem "done", then your tests have scheduled something to happen (asynchronously) and haven't cleaned up after themselves properly. Did you leave a socket open?

[dhensby]

I'm going to close this because I don't think there's much we can do about this, it's a developer expectation problem and not a bug.

If I've mis-understood, please let me know

[manu741]

Fixed in 4.0.4. Anyway, the fix doesn't close the process when all connections are closed. You need to explicitly call pool.close().

pool.end() worked for me

[gsaravanakumar932]

Connection Pool is not working, i am using the latest tedious 6.6.0 as well as mssql npm 6.0.0, but still i am facing the same issue, could anybody help me to fix this, any issue in my below code:

if i close pool.close() in finally, after that it is not connecting again, when the request comes from front end:

const connPool = new sql.ConnectionPool(dbConfig).connect();
const executeSql = function(query, callback) {
connPool
.then((pool) => {
return pool.request().query(query);
})
.then(result => {
return callback(result.recordset);
})
.catch(err => {
callback(null, err);
});
};

[dhensby]

@gsaravanakumar932 it seems like the pool is working. You're able to create a pool, connect to the DB, execute queries and close it.

If you close the pool and then continue to attempt to use it, then you will have problems because the pool will be closed.

You need to manage your connection pool better, either rely on the global pool we provide in sql.connect() or you'll need to create a new pool if the current one is closed.

[gsaravanakumar932]

@gsaravanakumar932 it seems like the pool is working. You're able to create a pool, connect to the DB, execute queries and close it.

If you close the pool and then continue to attempt to use it, then you will have problems because the pool will be closed.

You need to manage your connection pool better, either rely on the global pool we provide in sql.connect() or you'll need to create a new pool if the current one is closed.

@dhensby

I am able to create connectionpool, but it is not closing auto, if the request is done.

Below is my express router example code,

router.get('/getCategoryDetails', cache5mins, (req, res) => {
	const query = shareQuery.getAllCategories(req);
	executeSql(query, function(data, err) {
		if (err) { res.status(500).send(err); return; }
		res.status(200).json(data);
	});
});

DBconfig,

const dbConfig = {
	user: process.env.DB_USER_ID,
	password: process.env.DB_PWD,
	server: process.env.DB_SERVER,
	database: process.env.DB_NAME,
	port: Number(process.env.DB_PORT),
	requestTimeout: 500000,
	options: {
		encrypt: false // Use this if you're on Windows Azure
	},
	pool: {
		min: 10,
		idleTimeoutMillis: 30000
	}
};

this is my common connectionpool code,

const pool = new sql.ConnectionPool(dbConfig)
const connPool = pool.connect();

pool.on('error', err => {
    // ... error handler
})

const executeSql = function(query, callback) {
		connPool
		.then((pool) => {
			return pool.request().query(query);
		})
		.then(result => {
			callback(result.recordset);
		})
		.catch(err => {
			callback(null, err);
		});
};

Could you please suggest me where to close the connection in the above code, as part of best practices ?
I havent find any full snippet with connection pool closing function in the documentation. Thats why i dont know where to add the pool.close();

[gsaravanakumar932]

If i write like below,

const executeSql = function(query, callback) {
	pool.connect()
		.then((pool) => {
			return pool.request().query(query);
		})
		.then(result => {
			callback(result.recordset);
		})
		.catch(err => {
			callback(null, err);
		}).finally(() => {
			pool.close();
		});
};

I am getting error like below,

TypeError: pool.connect(...).then(...).then(...).catch(...).finally is not a function                                                      
    at executeSql (C:\adobe\smartspend-qa\ss-app-server\dist\app.js:16134:15)                                                              
    at ./src/routes/user-profile/user-profile.route.ts.router.get (C:\adobe\smartspend-qa\ss-app-server\dist\app.js:15717:5)               
    at Layer.handle [as handle_request] (C:\adobe\smartspend-qa\ss-app-server\node_modules\express\lib\router\layer.js:95:5)               
    at next (C:\adobe\smartspend-qa\ss-app-server\node_modules\express\lib\router\route.js:137:13)                                         
    at Route.dispatch (C:\adobe\smartspend-qa\ss-app-server\node_modules\express\lib\router\route.js:112:3)                                
    at Layer.handle [as handle_request] (C:\adobe\smartspend-qa\ss-app-server\node_modules\express\lib\router\layer.js:95:5)               
    at C:\adobe\smartspend-qa\ss-app-server\node_modules\express\lib\router\index.js:281:22                                                
    at Function.process_params (C:\adobe\smartspend-qa\ss-app-server\node_modules\express\lib\router\index.js:335:12)                      
    at next (C:\adobe\smartspend-qa\ss-app-server\node_modules\express\lib\router\index.js:275:10)                                         
    at Function.handle (C:\adobe\smartspend-qa\ss-app-server\node_modules\express\lib\router\index.js:174:3)                               

[dhensby]

1. What version of node are you using? Promise.finally() was added in node 10, so you should only get the TypeError: ... .finally is not a function if you are not using a version of node that has a finally implementation you'll need to use another promise library like Bluebird
2. you are holding a shared connection pool in memory as a constant. Once that pool is closed you can't reconnect to it (I don't think), so you should only close that pool when your application is stopping, not when you've executed a single query. The point of the pool is to remain persistent and used when you need to run a query instead of being opened and closed on every query you make.

As a comment, your callback arguments are non-standard. The conventional way to use a callback is:

(error, ...args) => {
    if (error) {
        // handle error case
    }
    // handle success case
}

You'd then do:

const connectionPool = (new sql.ConnectionPool(config)).connect();
function executeSql(query) {
    return connectionPool.then(pool => {
        return pool.request().query(query);
    }).then(result => {
        setImmediate(callback, null, result);
    }).catch(error => {
        setImmediate(callback, error);
    });
}

However, you're mixing promises and callbacks for some reason. The mssql library supports callbacks (though this is still a not very nice mix of callbacks and promises):

const connectionPool = (new sql.ConnectionPool(config)).connect();

function executeSql(query) {
    connectionPool.then(pool => {
       pool.request().query(query, callback);
    }).catch(callback);

or you can use promises in your express router:

router.get('/getCategoryDetails', cache5mins, (req, res) => {
	const query = shareQuery.getAllCategories(req);
	executeSql(query).then(data => {
            res.status(200).json(data);
        }).catch(err => {
            res.status(500).send(err);
	});
});

and

const connectionPool = (new sql.ConnectionPool(config)).connect();
function executeSql(query) {
    return connectionPool.then(pool => {
        return pool.request().query(query);
    });
}

lastly, because you're reusing variable names, you're making things less clear and your call to pool.close() in the finally callback is not going to work because you're calling it on the pool connection promise and not on the pool itself. Therefore you need to call pool.then(pool => pool.close()); in the finally.

[gsaravanakumar932]

@dhensby awesome. This is expected.