There's no timer for waiting the server's reply to a request, is it? If it were, the fix would have worked just fine.

I'm not sure I'm following here. Whatever you think the right solution is, please update the PR with it 👍

I'm talking about the simple example at https://github.com/mysqljs/mysql#introduction. It exits prematurely because there's nothing waiting for the answer to the request, now that the socket is not anymore a reason for the event loop to stay alive.

Ah, does that mean you need to update the README in this PR? Also, would that make this change not backwards-compatible with the code out there? If so, can you write up a migration guide we can include to help people migrate their code?

I'm entirely against putting on users' shoulders another thing to care for. The timer I'm talking about must be implemented in this module and any pending request should have an accompanying setTimeout() call.
If a request is not answered in a configurable time, then issue a timeout error. That's how the Elasticsearch connector works, for example.

I believe that if you point @mihai1voicescu to the right place in your code, where the pairing of responses to requests is made, he'll do the necessary changes.

To be honest, this is not exactly backwards compatible, as people with slower databases or more complicated queries will have to care for this new configuration parameter, no matter what default value you'd choose for it.

Makes sense. The pairing is done in the Protocol.js file 👍 and the main execution is handled by Sequence.js

After some thinking I came to the conclusion that it's better to ref the socket when the associated command queue has items and unref it when the queue is empty or a fatal error occurs. This will make the code be as backwards compatible as it can. I will make a pull request but please double check that I covered all the possible cases.

All the tests pass now. Hope it's OK.

Maybe have a look in the README.md to check if the new instructions are clear.

Awesome, thank you! I tried it out and after playing around with the changes and through they were good, suddenly my MySQL user was locked out. Digging into it, it seems that this change is causing the https://dev.mysql.com/doc/refman/5.7/en/server-status-variables.html#statvar_Aborted_clients variable to constantly increase, and eventually hits a block limit on a user. Is there a way to avoid that? I wouldn't what this module to cause account to get locked.

You can query your aborted variable with SHOW GLOBAL STATUS LIKE "aborted%". This should never go up in a functional environment, and with the into code it does, but that's because it's explicitly calling connection.end(), but the whole point of this PR is so users never need to call connection.end() any longer. But if not calling it will lock out their accounts, then that makes it still required at this point.

I'm not sure what the correct way forward is here, and here are some of my thoughts, but I'll leave the solution up to you as the expert here 👍 :

(1) Maybe just add warnings all over the docs that if you don't call connection.end() it will cause an error logged on your MySQL server for aborted clients and can reach error limits for accounts and cause lockouts.
(2) Maybe the unref behavior can be opt-in and then describe the potential pitfalls of enabling it and let the users choose if they want to unref or not with the default being as-is today (which also doesn't cause errors on the MySQL server from bad disconnects).
(3) Something else (I'll try to think more)

P.S. it looks like some of the CI builds are failing from the .unref calls.

The CI builds fail because in 0.6 and 0.8 apparently there was no unref method. Those versions are pretty old so I don't think this is a problem. Maybe just update the package JSON?

  "engines": {
    "node": ">= 0.10"
  },

As for the locking out part you are right, it's a pretty big problem but I have a very good solution.
The is an event named beforeClose on process. It is called when the server is going to shutdown.
We can remember all the connections that have not been closed, and close them there.
I will take in account older versions too and use a plain array for them. For the rest I will use Set for efficiency.

The CI builds fail because in 0.6 and 0.8 apparently there was no unref method. Those versions are pretty old so I don't think this is a problem. Maybe just update the package JSON?

Ah. So that would require a new major version of this library. That's OK if that really is necessary. We can queue up a 3.0 branch you can target for this change, then 👍 Not sure when a new major version will get released, though. Being the very very base mysql driver module, we support as many versions as absolutely possible, otherwise users don't have anything they can use.

As for the locking out part you are right, it's a pretty big problem but I have a very good solution.
The is an event named beforeClose on process. It is called when the server is going to shutdown.
We can remember all the connections that have not been closed, and close them there.

Ah, didn't know about beforeClose. That sounds like a reasonable solution to me 👍

I ran into some problems on the Pool side in the tests. First of all I get this warning:

Calling conn.end() to release a pooled connection is deprecated. In next version calling conn.end() will be restored to default conn.end() behavior. Use conn.release() instead.

Second a test fails:

 [root@v2 mysql-github]# FILTER="test-double-release" npm test

> [email protected] test /root/mysql-github
> node test/run.js

1..1
not ok 1 test/unit/pool/test-double-release.js
  /root/mysql-github/lib/protocol/Parser.js:80
          throw err; // Rethrow non-MySQL errors
          ^

  Error: ER_QUERY_INTERRUPTED: Interrupted unknown query
      at Query.Sequence._packetToError (/root/mysql-github/lib/protocol/sequences/Sequence.js:52:14)
      at Query.ErrorPacket (/root/mysql-github/lib/protocol/sequences/Query.js:77:18)
      at Protocol._parsePacket (/root/mysql-github/lib/protocol/Protocol.js:281:23)
      at Parser.write (/root/mysql-github/lib/protocol/Parser.js:76:12)
      at Protocol.write (/root/mysql-github/lib/protocol/Protocol.js:39:16)
      at Socket.<anonymous> (/root/mysql-github/lib/Connection.js:128:28)
      at Socket.<anonymous> (/root/mysql-github/lib/Connection.js:541:10)
      at emitOne (events.js:116:13)
      at Socket.emit (events.js:211:7)
      at addChunk (_stream_readable.js:263:12)
      --------------------
      at Protocol._enqueue (/root/mysql-github/lib/protocol/Protocol.js:145:48)
      at PoolConnection.query (/root/mysql-github/lib/Connection.js:237:25)
      at /root/mysql-github/test/unit/pool/test-double-release.js:16:10
      at Handshake.onConnect (/root/mysql-github/lib/Pool.js:64:7)
      at Handshake.<anonymous> (/root/mysql-github/lib/Connection.js:541:10)
      at Handshake._callback (/root/mysql-github/lib/Connection.js:507:16)
      at Handshake.Sequence.end (/root/mysql-github/lib/protocol/sequences/Sequence.js:88:24)
      at Handshake.Sequence.OkPacket (/root/mysql-github/lib/protocol/sequences/Sequence.js:97:8)
      at Protocol._parsePacket (/root/mysql-github/lib/protocol/Protocol.js:281:23)
      at Parser.write (/root/mysql-github/lib/protocol/Parser.js:76:12)

From what I can tell, the problem is that when the server shuts down, releases are generated for the respective ended connections resulting in duplicate releases. Unfortunately I have no idea how to fix this 😁

Another weird thing is that running the tests still increases the

+------------------+-------+
| Variable_name    | Value |
+------------------+-------+
| Aborted_clients  | 3102  |
| Aborted_connects | 72    |
+------------------+-------+

But my simple example does not.

• Does the Pool shutdown gracefully?
• If so, how does he do that? I don't see any calls to Connection#end.
• If this is going on 3.0 do I still push on master or create a new PR?

Index: lib/Connection.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- lib/Connection.js	(date 1523452026000)
+++ lib/Connection.js	(date 1523461415000)
@@ -10,11 +10,47 @@
 
 module.exports = Connection;
 Util.inherits(Connection, Events.EventEmitter);
+
+var addGracefulShutdown, removeGracefulShutdown, gracefulShutdowns;
+
+if (typeof Set === 'undefined') {
+  // backwards compatibility
+  gracefulShutdowns = [];
+  addGracefulShutdown = gracefulShutdowns.push.bind(gracefulShutdowns);
+  removeGracefulShutdown = function removeGracefulShutdown(fn) {
+    var index = gracefulShutdowns.indexOf(fn);
+    gracefulShutdowns.splice(index, 1);
+  };
+  process.once('beforeExit', function () {
+    for (var i = 0; i < gracefulShutdowns.length; i++) {
+      gracefulShutdowns[i]();
+    }
+  });
+}
+else {
+  // eslint-disable-next-line no-undef
+  gracefulShutdowns = new Set();
+  addGracefulShutdown = gracefulShutdowns.add.bind(gracefulShutdowns);
+  removeGracefulShutdown = gracefulShutdowns.delete.bind(gracefulShutdowns);
+
+  process.once('beforeExit', function () {
+    gracefulShutdowns.forEach(function (gracefulShutdown) {
+      gracefulShutdown();
+    });
+  });
+}
+
 function Connection(options) {
   Events.EventEmitter.call(this);
 
   this.config = options.config;
 
+  this._gracefulShutdown = function () {
+    this.end();
+  }.bind(this);
+
+  addGracefulShutdown(this._gracefulShutdown);
+
   this._socket        = options.socket;
   this._protocol      = new Protocol({config: this.config, connection: this});
   this._connectCalled = false;
@@ -233,6 +269,7 @@
   // create custom options reference
   opts = Object.create(opts || null);
 
+  removeGracefulShutdown(this._gracefulShutdown);
   if (opts.timeout === undefined) {
     // default timeout of 30 seconds
     opts.timeout = 30000;
@@ -247,6 +284,7 @@
   this._implyConnect();
   this._socket.destroy();
   this._protocol.destroy();
+  removeGracefulShutdown(this._gracefulShutdown);
 };
 
 Connection.prototype.pause = function() {
Index: package.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- package.json	(date 1523452026000)
+++ package.json	(date 1523461415000)
@@ -35,7 +35,7 @@
     "index.js"
   ],
   "engines": {
-    "node": ">= 0.6"
+    "node": ">= 0.10"
   },
   "scripts": {
     "lint": "eslint .",

Any news 😄 ?

Sorry I kind if forgot about this PR. I will take a look to answer your questions this evening my time. Do you know why the CI is failing for this? We wouldn't be able to merge with a failing build normally, at least