Event loop not draining with promises - 4.x - Pool Connections & Timeout evictions

[Shockolate]

Hello,

I'm experiencing new behavior with the mssql package (4.x) where the Event Loop is not draining despite being wrapped in promises.
I did not experience this behavior in 3.3.
The 3.3 code below would successfully retrieve the data and drain the Event Loop. The 4.x code still hangs.

I don't know if just don't understand the Promise paradigm, or if there was something significantly breaking that I didn't consider in v3.x -> v4.x

My 4.x code:

persistence.js

const sql = require('mssql');
function connectToDatabase(execute) {
  return configLoader.getConfig()
  .then(sql.connect)
  .then(execute)
  .catch((error) => {
    throw new Error(sqlErrorParser(error));
  });
}

function getEntityById(entityId, tenant) {
  return connectToDatabase(
    pool => (
      selectEntity(entityId, tenant, pool))
  );
}

// selectEntity() is the same as 3.3

My 3.3 code:

persistence.js

const sql = require('mssql');
const ConnectionManager = require('./connectionManager');
const connectionManager = new ConnectionManager();

function connectToDatabase(execute) {
  return new Promise((resolve, reject) => {
    connectionManager.getDatabaseConnection()
    .then(execute)
    .then(resolve)
    .catch(error => reject(sqlErrorParser(error)));
  });
}

function getEntityById(entityId, tenant) {
  return connectToDatabase(
    connection => (
      selectEntity(entityId, tenant, connection))
  );
}

function selectEntity(entityId, tenant, connection) {
  const getEntityRowRequest = constructEntityTenantRequest(entityId, tenant, connection);
  const getAliasRowsRequest = constructEntityTenantRequest(entityId, tenant, connection);
  const getMetadataRowsRequest = constructEntityTenantRequest(entityId, tenant, connection);

  const queries = [
    getEntityRowRequest.query(
        'SELECT EntityId, Tenant, CreatedOn, UpdatedOn ' +
        'FROM Entity WHERE EntityId=@entityId AND Tenant=@tenant'),
    getAliasRowsRequest.query(
        'SELECT AliasKey, Value FROM Alias WHERE Tenant=@tenant AND EntityId=@entityId'),
    getMetadataRowsRequest.query(
        'SELECT MetaKey, Value FROM Metadata WHERE Tenant=@tenant AND EntityId=@entityId'),
  ];

  return Promise.all(queries).then(constructEntityFromQueryResult);
}


function constructEntityTenantRequest(entityId, tenant, connection) {
  const request = new sql.Request(connection);
  request.input('entityId', entityIdSqlType, entityId);
  request.input('tenant', tenantSqlType, tenant);
  return request;
}

function constructAliasTenantRequest(alias, tenant, connection) {
  const request = new sql.Request(connection);
  request.input('alias', aliasValueSqlType, alias);
  request.input('tenant', tenantSqlType, tenant);
  return request;
}

function constructMetadataRequest(entityId, tenant, metadataKey, metadataValue, connection) {
  const request = constructEntityTenantRequest(entityId, tenant, connection);
  request.input('metadataKey', metaKeySqlType, metadataKey);
  request.input('metadataValue', metaValueSqlType, metadataValue);
  return request;
}

connectionManager.js

const sql = require('mssql');
const ConfigLoader = require('./configLoader');
const databaseConfigKey = 'Secret.json';

class ConnectionManager {
  constructor() {
    this.cachedConnection = null;
    this.dbConfig = new ConfigLoader(databaseConfigKey);
  }

  getDatabaseConnection() {
    return new Promise((resolve, reject) => {
      if (this.cachedConnection) {
        resolve(this.cachedConnection);
      } else {
        this.dbConfig.getConfig()
        .then((config) => {
          this.cachedConnection = new sql.Connection(config);
          this.cachedConnection.connect()
          .then(() => {
            resolve(this.cachedConnection);
          },
          (error) => {
            log.error(`Connection error: ${error}`);
            reject(error);
          });
        })
        .catch(reject);
      }
    });
  }
}

module.exports = ConnectionManager;

[Shockolate]

Update:
In 3.3.0, I noticed that the idle connections were getting evicted as expected after the time specified by pool.idleTimeoutMillis.
However, in 4.0.4, I noticed that either none of the connections were getting evicted, or something is still registered in the event loop.

@patriksimek Did 4.0.4 fix this?

3.3.0 Config

{
    "user": "DatabaseUser",
    "password": "DatabasePassword",
    "server": "DatabaseServer",
    "database": "MyDatabase",
    "connectionTimeout": 10000,
    "requestTimeout": 45000,
    "pool": {
      "max": 10,
      "min": 0,
      "idleTimeoutMillis": 250
    }
}

4.0.4 Config

{
    "user": "DatabaseUser",
    "password": "DatabasePassword",
    "server": "DatabaseServer",
    "database": "MyDatabase",
    "connectionTimeout": 10000,
    "requestTimeout": 45000,
    "pool": {
      "max": 10,
      "min": 0,
      "idleTimeoutMillis": 50,
      "evictionRunIntervalMillis": 5,
      "softIdleTimeoutMillis": 5
    },
	"options": {
		"debug": {
			"packet": true
		}
	}
}

[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.

I'm fairly sure this could have been fixed by #683.