Pool leaking connections

[seangarner]

I need to code up a minimal test case but I'm finding that the connection pool leaks connections that are released if changeUser has been called on it during the lifetime of the connection.

When release is called on the connection it appears that it gets removed from the pool in so much that it's no longer in pool._allConnections or pool._freeConnections. But the connection must live on somewhere because the tcp connection stays open. I'm using the crude netstat -lanp | grep 3306 | wc -l to count the connections, and the number grows by the number of connections got from the pool.

If using the connection without calling changeUser then it returns the connection to the pool as expected.

[seangarner]

Only appears to be a bug in 2.3.0 and 2.3.1, it works as expected in 2.2.0.

[seangarner]

Same goes for master SHA: bce163c

[dougwilson]

Wow. Thanks for the report! I'll get a fix out ASAP. When a connection is marked to purge, we should go ahead and close the connection when you go to return it to the pool instead of leaking it.

[seangarner]

Thanks. As an aside why would the connection be marked to purge in this scenario? Unless I've misunderstood the docs that should only happen when I call destroy, right? (or end in a future version when the old end behaviour is fully deprecated).

Or are you saying using changeUser somehow prevent reuse of a connection?

[dougwilson]

Or are you saying using changeUser somehow prevent reuse of a connection?

It is because if you, say, have a pool where all the connections are user bob, but you pull off a connection and change it to mary sometimes. If you were able to release that connection back into the pool, parts of your code will randomly get the mary connection. To be safe, the pool will not re-accept connections when the state has been changed.

Another reason is that when you use changeUser, the session state is erased on the server-side. People setup connection state using pool.on('connection', fn), but now a random connection will not have the correct state any longer.

[seangarner]

That makes a lot of sense for that use case. I wonder, would you accept a PR to make that behaviour configurable?

I have an app where I have a small number of servers which each has a large number of databases each for which I have unique credentials. I'd like to be able to have 1 pool per server and be able to grab a connection in an express middleware and always immediately change the user and db on the connection before passing it onto the next middleware/route handler.

[dougwilson]

I'd like to be able to have 1 pool per server and be able to grab a connection in an express middleware and always immediately change the user and db on the connection before passing it onto the next middleware/route handler.

Perhaps you should be using the PoolCluster?

[dougwilson]

be able to grab a connection in an express middleware and always immediately change the user and db on the connection

If you didn't know, the changeUser operation is very slow, as it completely reestablishes a connection, so it is not the best thing to do before every request. You could use PoolCluster to add a pool per user/db, unless you are saying that the user is determined per request (i.e. you have logged in users and those users have a corresponding MySQL user)?

[seangarner]

Yes, it can change per request depending on which group the user belongs to and which "context" they are currently viewing. A db host is multi-tenancy but our apps users can have access to multiple dbs. We'd like to keep using the mysql users to enforce restrictions on users per db because of the multi-tenanted nature of it.

Previously we were managing a pool of pools whereby we kept 1 pool per host/db/user combo, but the drawback to that was the vast number of connections that would be kept open on a single db host. Add multiple instances of an app server (horizontal scaling) to the mix and suddenly the db has a huge number of open connections. If I've understood correctly the PoolCluster would have the same impact.

I guess we can always write an alternative pool manager to do it :)

[dougwilson]

Right. So I was asking because it doesn't really sound like you are using the pooling anyhow. The ejection will (mostly) just work fine with what you are trying to achieve. I assume you are only even using the pool (instead of just createConnection) to enforce a max number of open connections? Doing an instant changeUser negates every other feature of the pool ;)

[dougwilson]

That makes a lot of sense for that use case. I wonder, would you accept a PR to make that behaviour configurable?

Sorry I didn't address this earlier. So, ejecting the connections after changeUser was really a quick-and-dirty solution to keep the pool's connections cleaner. What I would love to have (so PR maybe?? :D) would be such that when you return a connection that has changeUser called on it, the pool will change the user back to what the pool is configured to and call the appropriate events so state can be set up again.

[seangarner]

But if you say that it's setting up a new connection every time you switch
user doesn't that mean this would not be any more efficient than waiting
until the next connection was requested from the pool?

I had assumed that it was using this
http://dev.mysql.com/doc/internals/en/com-change-user.html so in effect you
had to pay the price of reset and auth but at least save on establishing a
new TCP connection.

I need to go out but I'll pick this up again in the morning and actually
start looking at the node-mysql code ;)

Thanks for the quick responses and fix for the bug, much appreciated.
On 29 May 2014 19:27, "Douglas Christopher Wilson" [email protected]
wrote:

That makes a lot of sense for that use case. I wonder, would you accept a
PR to make that behaviour configurable?

Sorry I didn't address this earlier. So, ejecting the connections after
changeUser was really a quick-and-dirty solution to keep the pool's
connections cleaner. What I would love to have (so PR maybe?? :D) would be
such that when you return a connection that has changeUser called on it,
the pool will change the user back to what the pool is configured to and
call the appropriate events so state can be set up again.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/833#issuecomment-44566528
.

[dougwilson]

so in effect you had to pay the price of reset and auth but at least save on establishing a new TCP connection.

That is correct. By "new connection" I meant the negotiation on the MySQL server and the connection session state there, rather than the raw TCP connection.

would not be any more efficient than waiting until the next connection was requested from the pool?

That's fine ;) Like I said, the purging of the connections that had chnageUser done on them was a quick-and-dirty solution to solve a problem with the pool. The real solution would be to change the user back when needed, of course :) I just haven't had the time to implement that, but PRs are always welcome!

Thanks for the quick responses and fix for the bug, much appreciated.

No problem :)

[seangarner]

So I just submitted a PR for the changeUser tweak.

I've also been thinking more about my use case and I'm thinking that whilst the PR would be an improvement (in so much that we're not re-establishing the TCP connection) there's still a ways to go. I was thinking on modifying the pool so during getConnection you could specify credentials then it would look through _freeConnections for a connection already matching the credentials sent therefore saving the changeUser if there's a connection in the pool that already has the credentials you desired.

If you didn't specify credentials to the getConnection then it's assumed you want the pools default creds therefore it'd looks for one of those. If in either case it didn't find a suitable connection then it'd take any available and call changeUser on it before releasing it to the callback.

But that seems like a fair bit of change in Pool and PoolConnection. Would you take a PR for something like that, or would I be better writing a new Pool implementation as a new module? Thoughts welcome.

[dougwilson]

Would you take a PR for something like that, or would I be better writing a new Pool implementation as a new module?

Maybe. Because MySQL allows for changing users and such, it may indeed be useful. Just because the pool is simply a pool of opaque connections now, doesn't mean it has to be :) Basically what you are suggesting is that the pool should pool on the properties that are fundamental to the connection itself (host, port, SSL, etc.) but would allow you to acquire connections based on the session of that connection (user, database, charset, etc.).

[seangarner]

Yep, pretty much. I think it should work for more use cases, albeit a slightly more complicated implementation.