Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ResourceRequest timed out #610

Closed
munir131 opened this issue Feb 20, 2018 · 74 comments · Fixed by #683
Closed

ResourceRequest timed out #610

munir131 opened this issue Feb 20, 2018 · 74 comments · Fixed by #683
Assignees
Milestone

Comments

@munir131
Copy link

Node: 6.9.1
configuration:

pool: {
    max: 100,
    min: 1,
    idleTimeoutMillis: 15000
}

I am getting this error when there is less traffic or there is gap between requests.

error:  { TimeoutError: ResourceRequest timed out
        at ResourceRequest._fireTimeout (/home/munir/node-workspace/server/node_modules/generic-pool/lib/ResourceRequest.js:62:17)
        at Timeout.bound (/home/munir/node-workspace/server/node_modules/generic-pool/lib/ResourceRequest.js:8:15)
        at ontimeout (timers.js:365:14)
        at tryOnTimeout (timers.js:237:5)
        at Timer.listOnTimeout (timers.js:207:5) name: 'TimeoutError' }
@labs20
Copy link

labs20 commented Feb 27, 2018

+1, and its random.

Please any help?

@glebson1
Copy link

glebson1 commented Mar 5, 2018

If you're using transactions in your calls it seems to be an issue. Improper usage of them makes pool get stacked. Try to get rid off them.

@munir131
Copy link
Author

munir131 commented Mar 5, 2018

@glebson1 Thanks for your reply. But, I am not getting how it is affecting to it?

@munir131
Copy link
Author

munir131 commented Mar 7, 2018

@patriksimek Can you give me any hint that can help?

@gentlerainsky
Copy link

I got a similar problem with ESOCKETTIMEOUT. It happened when I deploy my app and my docker install the latest version of tedious (v2.3.1, I think) as its dependency. When I ran my batch jobs which used quite a large and complex query, it made a lot of timeout which didn't happen before in the previous version. While I don't know how write reproduce steps without using the same database and query, My current solution is to force its dependency to install tedious (v2.1.1) using npm-shrinkwrap.json that I made from my local repo.

@munir131
Copy link
Author

munir131 commented Mar 8, 2018

@gentlerainsky Thanks. I will try it.

@munir131
Copy link
Author

@gentlerainsky I tried by using tedious (v1.x) But still getting resourceRequest timeout issue.

@anton-bot
Copy link
Contributor

Sequelize fixed this bug. Please just reference their pull request. This is a huge issue that prevents normal operation!

sequelize/sequelize#8815

@munir131
Copy link
Author

@catcher-in-the-try They did same thing which is already done by some developers in pending PR(coopernurse/node-pool#198 & coopernurse/node-pool#221) of generic-pool. So, looks like there is issue in generic-pool

@JankiGadhiya
Copy link

Guys, Any updates in this? My production is heading to a big event today and I am in a big mess because of this issue.

@labs20
Copy link

labs20 commented Mar 16, 2018

I had a really hard time on my customer with this too. Managed to reduce the damage somehow with the config below, but its urgent to fix this one.
pool: { max: 100000, min: 1, idleTimeoutMillis: 50, evictionRunIntervalMillis: 5, softIdleTimeoutMillis: 5 }

@munir131
Copy link
Author

@labs20 Are you not facing more issue with this configuration? I think 50ms timeout will create this issue more frequently. Also, It will increase chances of this error.

@labs20
Copy link

labs20 commented Mar 16, 2018

Well, at least on my customer server it seems to have reduced the problem.

@munir131
Copy link
Author

@catcher-in-the-try @labs20 It is working fine for me after I used https://github.com/munir131/node-mssql

@anton-bot
Copy link
Contributor

@munir131 I couldn't figure out how to use, it doesn't seem to be documented or published to NPM.

@munir131
Copy link
Author

munir131 commented Mar 20, 2018

@catcher-in-the-try "mssql": "https://github.com/munir131/node-mssql/tarball/7227cdffbf66ca2e3dc31b150d9dd2c044c88bce"
Add this in your package.json instead of version in dependencies.
It is same as this library. Difference is I am ensuring that there should be at least one connection available when there is pending request for resource.

@yss14
Copy link

yss14 commented Apr 18, 2018

Would be nice to have an official fix of this issue. I encountered the same problem on our production servers and it's pretty annoying...

@anton-bot
Copy link
Contributor

anton-bot commented Apr 19, 2018

I ended up doing this:

  • Counter for SQL errors
  • Timer job runs every second to check if there have been more than two errors recently
  • If so, reestablish the connection by overwriting the instance of mssql with a new one
  • All the SQL operations are requeued in case of failure, so they don't get lost and eventually complete

@yss14
Copy link

yss14 commented Apr 19, 2018

@catcher-in-the-try What do you mean by If so, reestablish the connection by overwriting the instance of mssql with a new one? Just reconnecting to the database via mssql and retry all failed queries?

@munir131
Copy link
Author

@catcher-in-the-try How did you accomplish All the SQL operations are re-queued in case of failure, so they don't get lost and eventually complete?

@munir131
Copy link
Author

I fixed this issue by fork https://github.com/coopernurse/node-pool library. It is not looking issue of node-mssql. But, I am not sure.

@yss14
Copy link

yss14 commented May 18, 2018

@munir131 So you just enforce npm to install your forked node-pool fix and it's working now without any TimeoutError: ResourceRequest timed out?

@jacqt
Copy link
Contributor

jacqt commented Jun 2, 2018

The issue for us was that that the _poolCreate will sometimes randomly return a promise that never resolves when under high load.

_poolCreate () {

This is a problem because of the way the generic node-pool counts how many TDS connections are potentially available. The generic pool adds the number of currently pending promises that the _poolCreate method returns to the number of potentially available TDS connections - see https://github.com/coopernurse/node-pool/blob/e73247728432f2a681f87b3228ccf10ebe67d5aa/lib/Pool.js#L629

As a result, whenever the _poolCreate method fails to return a promise that resolves, the generic node-pool will over-estimate the number of potentially available TDS connections, and will wait indefinitely for the promise returned by the _poolCreate to resolve. (https://github.com/coopernurse/node-pool/blob/e73247728432f2a681f87b3228ccf10ebe67d5aa/lib/Pool.js#L218)

The simple fix for this was to just add the following lines to the tedious.js file:

master...jacqt:master

I don't think this is the best way to encode this logic but perhaps this will be helpful to other people in this thread

@spearmootz
Copy link

there was an alleged fix for this but i still get the error :(

@dreaganluna
Copy link

I'm also getting this error. This affects our production environment rather badly every other 2 days (and the we have to restart our service). Is there any more news on this?

@Spragalas
Copy link

Is this going to be fixed any time soon ? It effects multiple users in theirs productions including mine as well, @patriksimek ?

@munir131
Copy link
Author

@yss14 Yes. I pointed to my forked version in which I fixed a generic-pool issue.

@jacqt
Copy link
Contributor

jacqt commented Jun 25, 2018

@munir131 it is not an issue in generic-pool, it is an issue with node-mssql. I have explained in more detail in my previous comment, and I have submitted a pull request with a simple fix for this.

learnmews added a commit to learnmews/loopback-connector-mssql that referenced this issue Aug 28, 2018
upgrade mssql dependency to 4.2.1. mssql package has been updated to resolve an issue related to resource timeout error. Loopback connector is dependent on this package and can face similar issue. 

tediousjs/node-mssql#610
tediousjs/node-mssql#683
@madal3x
Copy link

madal3x commented Sep 5, 2018

Hello,

I made some tests (on 4.2.1) and it seems that i cannot have the script recover after the database had errors for more than a while (1 min in my case). I've put at the end of this message the two files with which i tested, and here are the tests and the results i had:

Test1 (without setting acquireTimeoutMillis; stop and start db after 10s)

  • start sql db
  • run node index.js
    script is outputting the word "tick" and the content of the db table
  • stop sql db and start sql db again after ~10seconds
    script is outputting the connection error and the word "tick"
    after sql db is started again, all the promises of the queries that were previously run will now resolve

Test2 (without acquireTimeoutMillis; stop and start db after >1min)

  • start sql db
  • run node index.js
    script is outputting the word "tick" and the content of the db table
  • stop sql db and start sql db again after >1min
    script is outputting the connection error and the word "tick"
    after sql db is started again, the script continues to output "tick" without resolving nor rejecting neither the previous promises, nor new promises

Test3 (with acquireTimeoutMillis set to 10000; stop and start db after >1min)

  • start sql db
  • run node index.js
    script is outputting the word "tick" and the content of the db table
  • stop sql db and start sql db again after >1min
    script is outputting the connection error, the word "tick"
    and the promises of queries that took longer than 10seconds to acquire a connection are rejected with the error: TimeoutError: ResourceRequest timed out
    after sql db is started again, the promises of queries continue to be rejected with the error: TimeoutError: ResourceRequest timed out

I would expect that in all these 3 types of tests, after the database is back up, to start resolving promises again. How could i achieve this, am i missing something?

index.js

process
    .on('unhandledRejection', (reason, promise) => {
        console.error(`unhandled rejection`);
        console.error(promise);
        console.error(reason);

        process.exit(1);
    })
    .on('uncaughtException', err => {
        const msg = `Uncaught exception: ${err.stack}\n exiting...`;

        console.error(msg);

        process.exit(1);
    });

const app = require('./app');
app(3);

app.js

const sql = require('mssql');

class Repo {
    constructor(connectionPool) {
        this.connectionPool = connectionPool;
    }

    async names() {
        const pool = await this.connectionPool;

        const queryRes = await pool
            .request()
            .query(
                'select name from names'
            );

        return queryRes['recordset'].map(r => r.name);
    }
}

function createConn() {
    const user = 'sa';
    const password = 'P@55w0rd';
    const server = 'localhost';
    const database = 'test';

    const pool = new sql.ConnectionPool({user, password, server, database, pool: {acquireTimeoutMillis: 10000}});

    pool.on('error', err => {
        console.error('onPoolError listener');
        console.error(err);
    });

    return pool.connect();
}

module.exports = (
    intervalSeconds,
    repo = new Repo(createConn())
) => {
    const main = () => run().catch(console.error);
    main();
    setInterval(
        main,
        intervalSeconds * 1000
    );

    setInterval(
        () => console.log('tick'),
        1500
    );

    async function run() {
        await repo.names()
            .then(res => console.log(`names res: ${res}`))
            .catch(err => console.error(`names err: ${err}`));
    }
};

@madal3x
Copy link

madal3x commented Sep 6, 2018

Opening a new issue, since reopening this one is not possible: #701

@madal3x
Copy link

madal3x commented Sep 7, 2018

Regarding the fix that was made by @willmorgan for this issue, https://github.com/tediousjs/node-mssql/pull/683/files#diff-740c6ca62b823be279cef39ece34cf7e

which rejects the promise for creating the pool. It looks to me that the method poolCreate is called when the connect method is called, so in the beginning. If then the connection was successful, poolCreate will return a resolved promise. And if later there are connection errors, that resolved promise cannot change, since a promise, resolved or rejected, is immutable.

@willmorgan
Copy link
Collaborator

To clarify, my understanding is that poolCreate is called any time a connection is made, not just when the pool itself is created. It may reject on the creation of a connection: if that's the case, the the pool library will try to create another.

That fix works for my team in production.

@madal3x
Copy link

madal3x commented Sep 7, 2018

The way i see it is that you connect when you instantiate the application, and afterwards you issue queries, and don't try to connect again.

When you say it's working for your team, can you tell me, is it working in the scenario that the database goes down and recovers? Also, could you please show a simplified code of how you are using it?

@willmorgan
Copy link
Collaborator

If I recall correctly, poolCreate is one of the "pool factory" methods. It calls the underlying Tedious _poolCreate method. If you read through, it should become clear that you're operating on singular connections rather than the pool itself: https://github.com/tediousjs/node-mssql/blob/master/lib/tedious.js#L194

We're using the pool configuration more or less as per the docs, so there's no special code to show there.

Regarding a total database outage, we haven't had one of those, but understanding how pool connection eviction works, as a guess, I'd expect to see RequestErrors due to timeouts for query operations on open connections that haven't yet been evicted. However just because a timeout occurs doesn't necessarily mean the connection should be evicted from the pool. Eventually you'd see lots of create operations to ensure a minimum size in the pool, and as it came up, those would eventually recover some sort of usable state.

@madal3x
Copy link

madal3x commented Sep 7, 2018

as i interpret the code, poolCreate is called when the ConnectionPool is instantiated. If connect() is successful, it will return a resolved Promise with the ConnectionPool. I cannot see any subsequent calls to connect, or reinstantiation of the ConnectionPool, for instance in query() methods.

The issue i encounter is that the library will continue returning timeout RequestError even after the database recovered. Maybe i didnt understand exactly what was the edge case that you tried to fix, can you tell me what edge case(s) you fixed by adding that reject in the poolCreate?

@willmorgan
Copy link
Collaborator

poolCreate is called when the ConnectionPool is instantiated

Yes, because when the connection pool starts it will start to populate the pool with connections. The first connect is there to test if the config is good.

the library will continue returning timeout RequestError even after the database recovered

That sounds plausible; my guess is that the mssql library might be trying to run the request through a connection to the database that existed prior to the unavailability.

If this is true then you might want to look at custom eviction logic to evict dead connections, or implement a more stringent validate handle (I tried this by running SELECT 1 and adding a short timeout to complete), to avoid borrowing them to begin with.

@madal3x
Copy link

madal3x commented Sep 9, 2018

Thanks, though I wasn't looking for an explanation on why poolCreate is called when it's called, i wanted to understand what is it that you fixed by adding that reject in there.

I managed to "hackfix" it so that it recovers by replacing the whole connection pool when there are errors. I had to do it in a timeout, since doing it immediately would somehow result in no on error handler attached and issueing unhandled rejections. Though this will lose previous issued queries. If someone would want to keep those, a queue would have to sit between the repo method and the connection pool.

const sql = require('mssql');

class Repo {
    constructor(cfg) {
        const self = this;

        self.config = cfg;
        self.timerNewConn = null;
        self.connectionPool = self.newConn();
    }

    newConn() {
        const self = this;

        const pool = new sql.ConnectionPool(self.config);
        pool.on('error', err => {
            console.error('onPoolError listener');
            //console.error(err);

            if (self.timerNewConn) {
                clearTimeout(self.timerNewConn);
            }

            self.timerNewConn = setTimeout(() => {
                pool.close().then(() => self.connectionPool = self.newConn());
            }, 7000)
        });

        return pool.connect(err => {
            if (err) {
                console.error('onPoolConnectError listener');
                console.error(err);

                if (self.timerNewConn) {
                    clearTimeout(self.timerNewConn);
                }

                self.timerNewConn = setTimeout(() => {
                    pool.close().then(() => self.connectionPool = self.newConn());
                }, 7000)
            }
        });
    }

    async names() {
        const pool = await this.connectionPool;

        const queryRes = await pool
            .request()
            .query(
                'select name from names'
            );

        return queryRes['recordset'].map(r => r.name);
    }
}

function connCfg() {
    const user = 'sa';
    const password = 'P@55w0rd';
    const server = 'localhost';
    const database = 'test';

    return {user, password, server, database/*, debug: true*/, pool: {
            min: 1,
            max: 5,
            acquireTimeoutMillis: 5000,
            evictionRunIntervalMillis: 500
        }};
}

module.exports = (
    intervalSeconds,
    repo = new Repo(connCfg())
) => {
    const main = () => run().catch(console.error);
    main();
    setInterval(
        main,
        intervalSeconds * 1000
    );

    async function run() {
        console.log("next");

        await repo.names()
            .then(res => console.log(`names res: ${res}`))
            .catch(err => console.error(`names err: ${err.stack}`));
    }
};

@madal3x
Copy link

madal3x commented Sep 9, 2018

Though your solution proposals @willmorgan sound plausible too.

@willmorgan
Copy link
Collaborator

Thanks, though I wasn't looking for an explanation on why poolCreate is called when it's called, i wanted to understand what is it that you fixed by adding that reject in there.

Cool, good luck.

@LoveMeWithoutAll
Copy link

LoveMeWithoutAll commented Oct 22, 2018

@dsbert That's interesting. I haven't seen an ETIMEOUT error crash our Node services yet but I'll keep an eye out for that. Do you have a stack trace you can share? If there's a defect in my PR then would be keen to fix...

I am also facing same error frequently. So I have updated node-mssql version to v4.2.2. I'll keep an eye out for that.

@madal3x
Copy link

madal3x commented Oct 22, 2018

There is a fix in tedious that would be nice to be taken into node-mssql to fix this issue: tediousjs/tedious#769

@madal3x
Copy link

madal3x commented Oct 22, 2018

Ah sorry, i see it was included in 4.2.2

@LoveMeWithoutAll
Copy link

@dsbert That's interesting. I haven't seen an ETIMEOUT error crash our Node services yet but I'll keep an eye out for that. Do you have a stack trace you can share? If there's a defect in my PR then would be keen to fix...

I am also facing same error frequently. So I have updated node-mssql version to v4.2.2. I'll keep an eye out for that.

I updated node-mssql version to v4.2.2. But I am facing same error too. Connection timeout.

@dhensby
Copy link
Collaborator

dhensby commented Oct 23, 2018

@LoveMeWithoutAll Can you provide some code that reproduces the error, the config and also details of the environment / platform you are running on, please?

@LoveMeWithoutAll
Copy link

LoveMeWithoutAll commented Oct 25, 2018

@LoveMeWithoutAll Can you provide some code that reproduces the error, the config and also details of the environment / platform you are running on, please?

@dhensby I have encountered connection timeout error even if upgraded node-mssql to v4.2.2. I couldn't reproduce the error immediately. But after running 1~2 days, the error has occured. Getting connection every 1 second may raise the error? I'm speculating so. At now, I changed timing of getting connection to only once at a process lifetime, and keeping an eye out for that.

  • environment/platform
  1. Windows Server 2008R2 standard
  2. MSSQL: Microsoft SQL Server 2014 - 12.0.4100.1 (X64)
    Apr 20 2015 17:29:27
    Copyright (c) Microsoft Corporation
    Enterprise Edition (64-bit) on Windows NT 6.3 (Build 9600: ) (Hypervisor)
  3. Node.js v8.12.0
  4. node-mssql v4.2.2

Error rasing code is below.

// ./config/dbConfig.json
{
  "user": "-",
  "password": "-",
  "server": "-",
  "port": 0,
  "database": "-",
  "options": {
    "encrypt": false
  },
  "pool": {
    "acquireTimeoutMillis": 15000
  }
}
// services/db.js
const sql = require("mssql")
const dbConfig = require("../config/dbConfig.json")
const { onDbError } = require("./log")
let connectionPool

sql.on("error", err => {
	sql.close()
	onDbError(err, "mssql error")
})

const getConnectionPool = async () => {
	try {
		connectionPool = await new sql.ConnectionPool(dbConfig).connect()
		connectionPool.on("error", async err => {
			await closeConnectionPool()
			onDbError(err, "getConnectionPool error")
		})
	} catch (err) {
		onDbError(err, "getConnectionPool error in catch")
	}
}

const closeConnectionPool = async () => {
	try {
		await connectionPool.close()
	} catch (err) {
		onDbError(err, "closeConnectionPool error")
	}
}

const getPushList = async () => {
	try {
		let result = await connectionPool.request()
			.input("GUBUN", sql.NVarChar, "ENGINE_PUSH")
			.input("CONDITION1", sql.NVarChar, "Android")
			.input("CONDITION2", sql.NVarChar, engineConfig.runningServer)
			.execute("b.dbo.TU_M_PUSH_XMLLIST")
		return result.recordset
	} catch (err) {
		onDbError(err, "getPushList")
		return []
	}
}
// ./server.js
// ...
const dbService = require("./services/db")
// ...
const sendPush = async () => {
	await dbService.getConnectionPool()
	dbService.getPushList()
		.then((msgList) => {
			fcmService.sendMsgList(msgList)
		})
		.catch(async (error) => {
			await dbService.closeConnectionPool()
			onServerError(error)
		})
		.finally(async () => {
			await  dbService.closeConnectionPool()
			setTimeout(() => {
				sendPush()
			}, 1000)
		})
}
// ...
sendPush()

error log is below

{ message: 'Error on getConnectionPool error in catch in db.js',
  level: 'error',
  timestamp: '2018-10-23 10:27:42',
  [Symbol(level)]: 'error',
  [Symbol(message)]: '{"message":"Error on getConnectionPool error in catch in db.js","level":"error","timestamp":"2018-10-23 10:27:42"}' }
{ ConnectionError: Failed to connect to 165.246.17.29:3361 in 15000ms
    at Connection.tedious.once.err (D:\push_engine\FcmPushEngine\node_modules\mssql\lib\tedious.js:237:17)
    at Object.onceWrapper (events.js:315:30)
    at emitOne (events.js:116:13)
    at Connection.emit (events.js:211:7)
    at Connection.connectTimeout (D:\push_engine\FcmPushEngine\node_modules\tedious\lib\connection.js:944:12)
    at Timeout._onTimeout (D:\push_engine\FcmPushEngine\node_modules\tedious\lib\connection.js:913:16)
    at ontimeout (timers.js:498:11)
    at tryOnTimeout (timers.js:323:5)
    at Timer.listOnTimeout (timers.js:290:5)
  code: 'ETIMEOUT',
  originalError: 
   { ConnectionError: Failed to connect to 165.246.17.29:3361 in 15000ms
    at ConnectionError (D:\push_engine\FcmPushEngine\node_modules\tedious\lib\errors.js:12:12)
    at Connection.connectTimeout (D:\push_engine\FcmPushEngine\node_modules\tedious\lib\connection.js:944:28)
    at Timeout._onTimeout (D:\push_engine\FcmPushEngine\node_modules\tedious\lib\connection.js:913:16)
    at ontimeout (timers.js:498:11)
    at tryOnTimeout (timers.js:323:5)
    at Timer.listOnTimeout (timers.js:290:5)
     message: 'Failed to connect to 165.246.17.29:3361 in 15000ms',
     code: 'ETIMEOUT' },
  name: 'ConnectionError',
  level: 'error',
  timestamp: '2018-10-23 10:27:42',
  [Symbol(level)]: 'error',
  [Symbol(message)]: '{"code":"ETIMEOUT","originalError":{"message":"Failed to connect to 165.246.17.29:3361 in 15000ms","code":"ETIMEOUT"},"name":"ConnectionError","level":"error","timestamp":"2018-10-23 10:27:42"}' }

@dhensby
Copy link
Collaborator

dhensby commented Oct 25, 2018

@LoveMeWithoutAll Something about your getConnectionPoolcode feels weird because you're always reassigning connectionPool with a new pool rather than making use of a pool that may have already been created...

Can you try changing getConnectionPool to be something more like:

const getConnectionPool = async () => {
+	if (connectionPool) {
+ 		return connectionPool;
+	}
	try {
		connectionPool = await new sql.ConnectionPool(dbConfig).connect()
		connectionPool.on("error", async err => {
			await closeConnectionPool()
			onDbError(err, "getConnectionPool error")
		})
	} catch (err) {
		onDbError(err, "getConnectionPool error in catch")
	}
}

There's a possibility you're just creating a connection pool every time and eventually hitting the maximum number of connections allowed on the server and so the new pool never connects again.

Can you monitor the number of active connections on the server and see if it is constantly increasing or remains stable?

@LoveMeWithoutAll
Copy link

LoveMeWithoutAll commented Oct 26, 2018

@dhensby Thank you for your guide! I changed code as you recommended. And now 3 different version apps is running on my each server.

  1. Old one(always reassigning connectionPool)
  2. New one as you recommended
  3. Another one(Getting connection once at starting app)

Until now, the number of active connections on the all servers are stable.

I'm keeping an eye out for that.

P.S.

  1. I added some code on your if statement
const getConnectionPool = async () => {
	try {
+               if (connectionPool && connectionPool.connected) return
		connectionPool = await new sql.ConnectionPool(dbConfig).connect()
		connectionPool.on("error", async err => {
			await closeConnectionPool()
			onDbError(err, "getConnectionPool error")
		})
	} catch (err) {
		onDbError(err, "getConnectionPool error in catch")
	}
}
  1. I used below query to check the number of active connections
SELECT PROGRAM_NAME, HOSTNAME, STATUS, COUNT(DBID) AS NumberOfConnections
FROM sys.sysprocesses
WHERE PROGRAM_NAME = 'node-mssql'
GROUP BY PROGRAM_NAME, HOSTNAME, STATUS

@dhensby
Copy link
Collaborator

dhensby commented Oct 26, 2018

  1. I added some code on your if statement

It shouldn't matter if the connectionPool is connected or not because it could be in the process of connecting when it's called and you'll still be creating many pools that you lose reference to and thus can't close...

perhaps you should just un-assign the variable in the catch block?

@LoveMeWithoutAll
Copy link

  1. I added some code on your if statement

It shouldn't matter if the connectionPool is connected or not because it could be in the process of connecting when it's called and you'll still be creating many pools that you lose reference to and thus can't close...

perhaps you should just un-assign the variable in the catch block?

@dhensby Thank you for your advice! I have agreed with you. I changed my code.

const getConnectionPool = async () => {
	try {
+		if (connectionPool) return
		connectionPool = await new sql.ConnectionPool(dbConfig).connect()
		connectionPool.on("error", async err => {
			await closeConnectionPool()
			onDbError(err, "getConnectionPool error")
		})
	} catch (err) {
+		connectionPool = null
		onDbError(err, "getConnectionPool error in catch")
	}
}

const closeConnectionPool = async () => {
	try {
		await connectionPool.close()
	} catch (err) {
+		connectionPool = null
		onDbError(err, "closeConnectionPool error")
	}
}

And so far, there is no connection problem in my app I am keeping an eye for 4 days!

@dhensby
Copy link
Collaborator

dhensby commented Oct 31, 2018

Fantastic

@LoveMeWithoutAll
Copy link

LoveMeWithoutAll commented Nov 14, 2018

@dhensby Finally I got a same error Failed to connect to 165.246.17.29:3361 in 15000ms after 6 days running. No too many connections were in there. So I edited my code. When error occured, the app would be restarted by process.exit() without any trying to reconnect on DB.

@dhensby
Copy link
Collaborator

dhensby commented Nov 22, 2018

@LoveMeWithoutAll ok, so is it now completely resolved for you by fixing your code?

@LoveMeWithoutAll
Copy link

@dhensby All problems is resolved by restarting app when error occured. Thank you for your kindness!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.