search

CodeFoodPixels / node-promise-mysql

A wrapper for mysqljs/mysql that wraps function calls with Bluebird promises.
MIT License
338 stars 64 forks source link

Reconnect feature cause an EventEmitter memory leak #127

Closed nathan818fr closed 4 years ago

nathan818fr commented 4 years ago

When used with connections pools, this module can cause an EventEmitter memory leak:

(node:9472) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 error listeners added to [PoolConnection]. Use emitter.setMaxListeners() to increase limit
MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 error listeners added to [PoolConnection]. Use emitter.setMaxListeners() to increase limit
    at _addListener (events.js:261:17)
    at PoolConnection.addListener (events.js:277:10)
    at PoolConnection.once (events.js:306:8)
    at addReconnectHandler (/.../node_modules/promise-mysql/lib/connection.js:138:16)
    at /.../node_modules/promise-mysql/lib/connection.js:41:17
    at tryCatcher (/.../node_modules/bluebird/js/release/util.js:16:23)
    at Promise._settlePromiseFromHandler (/.../node_modules/bluebird/js/release/promise.js:517:31)
    at Promise._settlePromise (/.../node_modules/bluebird/js/release/promise.js:574:18)
    at Promise._settlePromiseCtx (/.../node_modules/bluebird/js/release/promise.js:611:10)
    at _drainQueueStep (/.../node_modules/bluebird/js/release/async.js:142:12)
    at _drainQueue (/.../node_modules/bluebird/js/release/async.js:131:9)
    at Async._drainQueues (/.../node_modules/bluebird/js/release/async.js:147:5)
    at Immediate.Async.drainQueues (/.../node_modules/bluebird/js/release/async.js:17:14)
    at processImmediate (internal/timers.js:439:21)
    at process.topLevelDomainCallback (domain.js:126:23)

Pool.getConnection create a new PoolConnection which call addReconnectHandler and add an error listener. But this listener is never removed and another will be added on the next Pool.getConnection call.

nathan818fr commented 4 years ago

Anyway the reconnect feature does not make any sense with connections Pools. They already have an reconnect feature built internally.

The best fix seem to add reconnect: false here: https://github.com/lukeb-uk/node-promise-mysql/blob/f2e72814d40146f3025b04c4b78d606c6aec8be2/lib/pool.js#L42:L46

Kosta-Github commented 4 years ago

I am facing the same issue @CodeFoodPixels is the proposed fix from @nathan818fr reasonable? and if so, could it be fixed, please?

CodeFoodPixels commented 4 years ago

@nathan818fr Where does it say they have a reconnect feature in the pools?

nathan818fr commented 4 years ago

Reconnect is a base feature of any pools system. Without reconnection a pool will be fundamentally broken. As you can see here, when acquiring a connection, if the connection is closed (or encounter any error):

Also note that when an existing connection is retrieved, a ping is made to check if the connection is still good.

--

In addition, currently, the reconnect feature of this module does not work with pools (so the fix will not even be a breaking change!). Explanation:

PBug90 commented 4 years ago

I can confirm what @nathan818fr wrote above. I get the following stack trace when a reconnect is attempted with a pool connection:

Error: connect ECONNREFUSED 127.0.0.1:3306
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1106:14)
    --------------------
    at Protocol._enqueue (/usr/src/app/node_modules/mysql/lib/protocol/Protocol.js:144:48)
    at Protocol.handshake (/usr/src/app/node_modules/mysql/lib/protocol/Protocol.js:51:23)
    at Connection.connect (/usr/src/app/node_modules/mysql/lib/Connection.js:119:18)
    at Promise (/usr/src/app/node_modules/promise-mysql/lib/connection.js:123:20)
    at Promise._execute (/usr/src/app/node_modules/bluebird/js/release/debuggability.js:313:9)
    at Promise._resolveFromExecutor (/usr/src/app/node_modules/bluebird/js/release/promise.js:488:18)
    at new Promise (/usr/src/app/node_modules/bluebird/js/release/promise.js:79:10)
    at connect (/usr/src/app/node_modules/promise-mysql/lib/connection.js:122:12)
    at PoolConnection.connection.once (/usr/src/app/node_modules/promise-mysql/lib/connection.js:144:13)
    at Object.onceWrapper (events.js:286:20)
    at PoolConnection.emit (events.js:203:15)
    at PoolConnection.EventEmitter.emit (domain.js:448:20)
    at PoolConnection.Connection._handleProtocolError (/usr/src/app/node_modules/mysql/lib/Connection.js:426:8)
    at Protocol.emit (events.js:198:13)
    at Protocol.EventEmitter.emit (domain.js:448:20)
    at Protocol._delegateError (/usr/src/app/node_modules/mysql/lib/protocol/Protocol.js:398:10)

This reconnect attempt is made after an initial connection succeeded with a given config object. At this point due to missing properties the underlying mysql library falls back to localhost and therefore can not reconnect successfully.

mytlogos commented 4 years ago

This may be what @nathan818fr described with 

* Then, the [reconnection](https://github.com/CodeFoodPixels/node-promise-mysql/blob/f2e72814d40146f3025b04c4b78d606c6aec8be2/lib/connection.js#L120) will fail because no host and port properties are available !

when the mysql server runs on localhost, or is this another issue altogether? I hope this is not a question which should land on SO

{ Error: ER_ACCESS_DENIED_ERROR: Access denied for user ''@'localhost' (using password: NO)
at Handshake.Sequence._packetToError (/app/node_modules/mysql/lib/protocol/sequences/Sequence.js:47:14)
at Handshake.ErrorPacket (/app/node_modules/mysql/lib/protocol/sequences/Handshake.js:123:18)
at Protocol._parsePacket (/app/node_modules/mysql/lib/protocol/Protocol.js:291:23)
at Parser._parsePacket (/app/node_modules/mysql/lib/protocol/Parser.js:433:10)
at Parser.write (/app/node_modules/mysql/lib/protocol/Parser.js:43:10)
at Protocol.write (/app/node_modules/mysql/lib/protocol/Protocol.js:38:16)
at Socket.<anonymous> (/app/node_modules/mysql/lib/Connection.js:91:28)
at Socket.<anonymous> (/app/node_modules/mysql/lib/Connection.js:525:10)
at Socket.emit (events.js:197:13)
at addChunk (_stream_readable.js:288:12)
at readableAddChunk (_stream_readable.js:269:11)
at Socket.Readable.push (_stream_readable.js:224:10)
at TCP.onStreamRead [as onread] (internal/stream_base_commons.js:145:17)
--------------------
at Protocol._enqueue (/app/node_modules/mysql/lib/protocol/Protocol.js:144:48)
at Protocol.handshake (/app/node_modules/mysql/lib/protocol/Protocol.js:51:23)
at Connection.connect (/app/node_modules/mysql/lib/Connection.js:119:18)
at Promise (/app/node_modules/promise-mysql/lib/connection.js:123:20)
at Promise._execute (/app/node_modules/bluebird/js/release/debuggability.js:313:9)
at Promise._resolveFromExecutor (/app/node_modules/bluebird/js/release/promise.js:488:18)
at new Promise (/app/node_modules/bluebird/js/release/promise.js:79:10)
at connect (/app/node_modules/promise-mysql/lib/connection.js:122:12)
at PoolConnection.connection.once (/app/node_modules/promise-mysql/lib/connection.js:144:13)
at Object.onceWrapper (events.js:285:13)
at PoolConnection.emit (events.js:202:15)
at PoolConnection.Connection._handleProtocolError (/app/node_modules/mysql/lib/Connection.js:426:8)
at Protocol.emit (events.js:197:13)
at Protocol._delegateError (/app/node_modules/mysql/lib/protocol/Protocol.js:398:10)
at Protocol.end (/app/node_modules/mysql/lib/protocol/Protocol.js:116:8)
at Socket.<anonymous> (/app/node_modules/mysql/lib/Connection.js:97:28)
at Socket.<anonymous> (/app/node_modules/mysql/lib/Connection.js:525:10)
at Socket.emit (events.js:202:15)
at endReadableNT (_stream_readable.js:1129:12)
at processTicksAndRejections (internal/process/next_tick.js:76:17)
code: 'ER_ACCESS_DENIED_ERROR',
errno: 1045,
sqlMessage: "Access denied for user ''@'localhost' (using password: NO)",
sqlState: '28000',
fatal: true }

This happens to me 'randomly', which may be the case if it is a reconnect issue.

CodeFoodPixels commented 4 years ago

Fixed with #129. Released as 4.1.1