Node-mysql2: Connection.connectTimeout is not cleared when connect error happens

Created on 6 Aug 2019 · 8Comments · Source: sidorares/node-mysql2

Timeout connectTimeout which is created in Connection constructor (node_modules/mysql2/lib/connection.js) is not cleared when error happens. It should be cleared for example in callback _handleNetworkError.

Run this without mysqld listening on 127.0.0.1:3306:
``` node js
let mysql = require("mysql2");
let connection = mysql.createConnection({});
connection.on("error", err => console.log(new Date(), "error", err))
process.on("exit", () => console.log(new Date(), "exit"));

Possible output:

2019-08-06T20:05:27.571Z error Error: connect ECONNREFUSED 127.0.0.1:3306
at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1056:14) {
errno: 'ECONNREFUSED',
code: 'ECONNREFUSED',
syscall: 'connect',
address: '127.0.0.1',
port: 3306,
fatal: true
}
2019-08-06T20:05:37.573Z exit

Notice that it takes 10 seconds to exit which is default for connect timeout.

Without error event handler on repl.it:
``` node js
let mysql = require("mysql2");
let connection = mysql.createConnection({});

Process crashed with: Error: connect ECONNREFUSED 127.0.0.1:3306
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1106:14)
Process crashed with: Error: Connection lost: The server closed the connection.
    at Socket.Connection.stream.on (/home/runner/node_modules/mysql2/lib/connection.js:88:31)
    at Socket.emit (events.js:198:13)
    at TCP._handle.close (net.js:606:12)
Process crashed with: Error: connect ETIMEDOUT
    at Connection._handleTimeoutError (/home/runner/node_modules/mysql2/lib/connection.js:171:17)
    at ontimeout (timers.js:436:11)
    at tryOnTimeout (timers.js:300:5)
    at listOnTimeout (timers.js:263:5)
    at Timer.processTimers (timers.js:223:10)
  ```

This change solves this problem:
Before:
``` node js
  _handleNetworkError(err) {
    // Do not throw an error when a connection ends with a RST,ACK packet
    if (err.errno === 'ECONNRESET' && this._closing) {
      return;
    }
    this._handleFatalError(err);
  }

After:
``` node js
_handleNetworkError(err) {
if (this.connectTimeout) {
Timers.clearTimeout(this.connectTimeout);
this.connectTimeout = null;
}
// Do not throw an error when a connection ends with a RST,ACK packet
if (err.errno === 'ECONNRESET' && this._closing) {
return;
}
this._handleFatalError(err);
}

Now it exits immediately:

2019-08-06T20:11:31.459Z error Error: connect ECONNREFUSED 127.0.0.1:3306
at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1056:14) {
errno: 'ECONNREFUSED',
code: 'ECONNREFUSED',
syscall: 'connect',
address: '127.0.0.1',
port: 3306,
fatal: true
}
2019-08-06T20:11:31.464Z exit
```

Maybe it should be cleared also in _handleFatalError.

Source

misos1

👍1

Most helpful comment

@dvisztempacct It is basically leak. What if someone uses really long timeout? These leaked timeouts can add up and cause real problems. It is like leaking files by not closing stream created by fs.createReadStream.

misos1 on 14 Aug 2019

👍2

All 8 comments

@misos1 Hi

Have you had any issues other than node.js taking extra time to exit?

Thanks!

dvisztempacct on 14 Aug 2019

misos1 on 14 Aug 2019

👍2

hi @misos1 , I think you are correct. Do you want to create PR with the fix?

sidorares on 15 Aug 2019

Yup, bumped into this issue today,
it seems it is correctly cleared in _handleTimeoutError

_handleTimeoutError() {
    if (this.connectTimeout) {
      Timers.clearTimeout(this.connectTimeout);
      this.connectTimeout = null;
    }
    this.stream.destroy && this.stream.destroy();
    const err = new Error('connect ETIMEDOUT');
    console.log(err.stack);
    err.errorno = 'ETIMEDOUT';
    err.code = 'ETIMEDOUT';
    err.syscall = 'connect';
    this._handleNetworkError(err);
  }