mysqljs / mysql

A pure node.js JavaScript Client implementing the MySQL protocol.
MIT License
18.27k stars 2.52k forks source link

connection.changeUser compromising the whole pool.getConnection() #2535

Closed TheVoid-0 closed 2 years ago

TheVoid-0 commented 2 years ago

This may be intentional, but if it is, i'd need some help. Basically what happens is that when you get a pool connection and then change it's user (i use this because i need to change the database) and a error occurs, like bad credentials for the new user, instead of only compromising the current connection that i've got, it compromises the whole pool, whenever i try to get another connection from the pool, the pool tries to use the same wrong credentials that i've passed to the connection.changeUser. This makes the pool unusable in my code because i can never get a connection again to be able to change user, in my understanding, changing the user on the connection should only affect that pooledConnection and not the whole pool.

I have one pool per server, and that server has many dbs, that's why i need to change the connection user from that specific pool, when some error occurs that makes that connection unusable, i use connection.destroy(), but this don't prevent the pool to keep trying to connect the new pool.getConnection() with the same wrong credentials i passed to the one i destroyed.

I would like to get some info if this behaviour is intentional, or if it really has this behaviour at all, maybe i'm fuc**ing things up somewhere... Thanks for helping in advance!

dougwilson commented 2 years ago

Hi @TheVoid-0 , sorry you are having trouble. That is definitely not intended behavior of this module. I did try real quick to reproduce the issue, but I was un able to. Would you mind putting together the code you are using that has this issue?

TheVoid-0 commented 2 years ago

Thanks for answering @dougwilson . i'm really sorry for bothering you. I have just realized that i actually use the node-mysql2 package and not this one. Even though the implementation of both packages are similar this behaviour may not be shared. I will try to come up with a repo to reproduce the issue and open the issue on the correct Github. But if you want to make sure that this package do not share this behaviour you can test again as well. Feel free to close this issue though. Sorry again.

Steps to reproduce the issue:

1- Attach a connection on pool creation using createPool without specifying the database. 2- get a connection from the pool. 3- change the user from that connection to an unexisting one, so it gives bad credential error 4- listen to changeuser errors and destroy the connection, then proceed to throw the error again and let express handle-it 5- if the error is thrown again that would stop any process on the given endpoint, now just proceed to call the same endpoint again, and it should throw an error on step 2, because the pool will try to get a connection using the bad credentials you gave to the other connection.

Later i will try to upload a reproducible repo, so feel free to wait and not waste any more of your time, but this is pretty much what my endpoint does.

I was able to fix it by surrounding the changeUser with a trycatch and then emitting an 'error' event on the connection so i could handle it in another function, this function releases the connection without destroying it. In my tests the issue was being caused by the connection.destroy inside the error handling function, once i left only the release the behaviour was now the one that is probrably intended, the new connection acquired from the pool now uses the default and i'm able to try changing the user again

Thanks again for answering.