release pool connection restores pool user configuration

[seangarner]

Discussed potential for this in #833 whereby a connection that is checked out from the pool then has changeUser called it would be marked for purging when it was released. Instead during release changeUser is called again to restore it back to the pool configuration before it's released back into the pool.

I've updated the test that covered restoreUser and all tests are passing locally, but I couldn't find a jshint or styleguide so I'm fully open to changes before merging.

[dougwilson]

Great! This is almost complete. It still needs to make sure that session state added in pool.on('connection', fn) is set back up as well in the restore user routine.

[seangarner]

Assuming I understood correctly that's done. However a Connections emit 3 types of event, error, drain and end. Is emitting connection with the same connection multiple times safe? A consumer might be listening to those events therefore they'd end up getting multiple callbacks fire for 1 event.

I guess I could clear those listeners in restoreUser but wanted to get your thoughts on it.

[dougwilson]

I'm not sure. The essence is that if a user does this:

pool.on('connection', function(conn) {
  conn.query('SET time_zone = "+00:00"')
})

That session state must be done on any connection that comes from pool.getConnection. conn.changeUser resets the connection, and would destroy that, which is why after restoring the user, those routines will need to be re-run on the connection.

[seangarner]

The PR now covers that scenario, but I notice that the tests are failing with node 0.8. I'll try to pick this up over the weekend as it's getting late here.

An alternative would be to have a resetConnection event on the pool which a consumer could listen on to achieve the same thing, but with possibly clearer intent.

[dougwilson]

An alternative would be to have a resetConnection event on the pool which a consumer could listen on to achieve the same thing, but with possibly clearer intent.

Yes, but that wouldn't be backwards-compatible and would delay this until whenever 3.0 came around :) To land in 2.x, it has to work with existing 2.x code :)

[dougwilson]

The PR now covers that scenario, but I notice that the tests are failing with node 0.8.

It's possible it just happened to fail on 0.8 because the modifications to the test just introduced some kind of race condition, rather than it not being compatible. You may need to look closer at possible race conditions in the test.

[seangarner]

Point well made ;) I'll investigate the tests then check to see if the readme needs modifying then get back to you.

[dougwilson]

This entire routine needs to be moved into the Pool. This is because restoring a user is a pool operation, not a connection operation. A user calling conn.restoreUser() doesn't make any sense.

[seangarner]

Yo @dougwilson I think this may be ready now. I moved the method onto Pool as request, fixed the test (looked to me like emit calling listeners happens async in 0.8 - best guess without looking) and update the readme and changes.

[dougwilson]

@seangarner thanks for the ping :) It looks great. I will likely have it merged tomorrow some time :D

[seangarner]

superseded by #1088.

/me makes a note to himself to find out how the rebase went so wrong