switch user back to pool default on connection release #1088

seangarner · 2015-05-12T07:04:24Z

reincarnation of #837...

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 you said it was ready to merge last year, but I guess something more important came up and I forgot to bump on it. Would be good to get this change in.

seangarner · 2015-05-12T22:06:12Z

Couldn't reproduce the test failure locally with any of those versions of node, but guessing it's a race condition perhaps exposed by windows? Not sure. Anyways I've pushed a fix so hopefully the next ones pass.

seangarner · 2015-05-13T15:09:32Z

@dougwilson the test failing now is completely unrelated to these changes, and doesn't happen when I run the tests locally against that version of node.

https://ci.appveyor.com/project/dougwilson/node-mysql/build/107/job/1vquubqcgptk422e#L219

test\unit\query\test-streaming-destroy.js

I'm going to assume that this is ok to merge unless you say otherwise.

dougwilson · 2015-05-13T15:13:17Z

Ok, so I just pushed a change to master. If it fails, then yes, it has nothing to do with your PR. If it doesn't fail, then even though it doesn't seem like it, it would seem very likely something in the PR is causing the failure.

seangarner · 2015-05-13T15:25:42Z

That test didn't fail the first time it ran: https://ci.appveyor.com/project/dougwilson/node-mysql/build/106/job/tjcdw1h46ll56nao#L234

And the only thing I changed in the last commit was an unrelated test. Perhaps it's intermittent?

dougwilson · 2015-05-13T15:30:49Z

It's possible your change may have introduced some kind of race condition? I'm not sure, as I haven't taken a hard look at the changes. When running on AppVeyor, there is typically high latency, which really brings out issues in code and tests where you're relying on async events to trigger in specific orders, which is one of the reasons I test on there.

dougwilson · 2015-05-13T15:37:03Z

lib/Pool.js

+    });
+    connection._purge = false;
+  } else {
+    callback();


This callback needs to be in a process.nextTick so that restoreUser is always async instead of sometimes async.

thanks for spotting that, fixed in 8b93ecb

dougwilson · 2015-05-13T17:46:01Z

Looks like that test failed on master, so you're changes are in the clear on that :)

seangarner · 2015-05-14T06:44:29Z

@dougwilson I've had a look at the travis failure and can't see how this change would have impacted that test.

https://travis-ci.org/felixge/node-mysql/jobs/62480666#L457

not ok 144 test/unit/query/test-streaming-destroy.js

  events.js:71
          throw arguments[1]; // Unhandled 'error' event
                         ^
  Error: This socket is closed.
      at Socket._write (net.js:519:19)
      at Socket.write (net.js:511:15)
      at FakeConnection.writeRow (/home/travis/build/felixge/node-mysql/test/FakeServer.js:389:18)
      at Timer.exports.setInterval.timer.ontimeout (timers.js:234:14)

If you disagree, I'd appreciate any suggestions on where to start looking.

seangarner · 2015-05-14T06:46:30Z

ha... I've just seen the latest master commit. I'll rebase this branch.

seangarner · 2015-05-15T08:03:24Z

@dougwilson tests are passing and I squashed the review comments ready for merge.

seangarner · 2015-05-21T16:03:21Z

Hi @dougwilson, I don't mean to pester; is there anything else you need me to do before this gets merged in? It'd be a shame to let it rot out in a PR again.

seangarner · 2015-06-22T09:55:00Z

Anything I can do to help get this merged?

dougwilson · 2015-07-02T05:04:40Z

I'm working on merging this now.

dougwilson · 2015-07-02T05:06:28Z

My biggest debate I've been having with myself is when we should call changeUser on the connection: when it is returned to the pool (like now), or when it is requested from the pool (in place of doing a ping on that connection).

dougwilson · 2015-07-02T05:07:26Z

Also, would be nice to have a new test specifically testing this, rather then only updating an existing test to account for the change.

dougwilson · 2015-07-02T05:16:04Z

test/unit/pool/test-change-user-eject.js

@@ -38,17 +44,20 @@ server.listen(common.fakeServerPort, function(err) {

  pool.getConnection(function(err, conn1) {
    assert.ifError(err);
-    assert.strictEqual(conn1.threadId, 3);
+    assert.ok(conn1.threadId === 1 || conn1.threadId === 2);
+    assert.strictEqual(conn1.config.user, 'user_1');


The test needs to run the query SELECT CURRENT_USER() to be sure the server-side actually got the change.

Fixed in 8989512

seangarner · 2015-07-02T09:48:05Z

Thanks for the feedback. Hoping to get time to start working on those changes later.

seangarner · 2015-07-02T18:24:25Z

would be nice to have a new test specifically testing this, rather then only updating an existing test to account for the change

The behaviour has changed from purging the connection from the pool, to switching user back to the pool config for the next time it's checked out. Perhaps it should be renamed? If not then some guidance on what the 2 separate tests are would be helpful.

dougwilson · 2015-07-10T00:52:38Z

lib/Pool.js

+  var config = connection.config;
+
+  // only when credentials have been changed from the pool defaults
+  if (config.user !== poolConfig.user || config.database !== poolConfig.database || config.password !== poolConfig.password || config.charset !== poolConfig.charset) {


there is no such thing as config.charset, only config.charsetNumber.

seangarner mentioned this pull request May 12, 2015

release pool connection restores pool user configuration #837

Closed

dougwilson self-assigned this May 12, 2015

dougwilson added the feature label May 12, 2015

dougwilson reviewed May 13, 2015
View reviewed changes

dougwilson force-pushed the master branch from 36d6449 to 59cd7e9 Compare May 13, 2015 18:33

seangarner force-pushed the no-disconnect-on-changeUser-return branch 2 times, most recently from 42f8430 to c458f83 Compare May 14, 2015 16:34

dougwilson force-pushed the master branch from 7c1d644 to 8ed9c50 Compare May 27, 2015 23:59

dougwilson reviewed Jul 2, 2015
View reviewed changes

seangarner force-pushed the no-disconnect-on-changeUser-return branch from c458f83 to b64b547 Compare July 2, 2015 17:03

seangarner added a commit to dynamicaction/node-mysql that referenced this pull request Jul 3, 2015

cherry pick changes from mysqljs#1088

80f297c

seangarner force-pushed the no-disconnect-on-changeUser-return branch 2 times, most recently from da76a2f to e45ab24 Compare July 3, 2015 21:31

seangarner added 2 commits July 3, 2015 22:31

switch user back to pool default on connection release

589a833

use purgeConnection instead of destroy on restoreUser error

1fcba72

seangarner force-pushed the no-disconnect-on-changeUser-return branch from e45ab24 to 1fcba72 Compare July 3, 2015 21:33

seangarner added 2 commits July 4, 2015 08:27

test connection is running user it says it is

8989512

fire changeUser when returned connection has different charset than pool

8a94f9b

dougwilson reviewed Jul 10, 2015
View reviewed changes

dougwilson closed this in f0dfda8 Jul 10, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

switch user back to pool default on connection release #1088

switch user back to pool default on connection release #1088

seangarner commented May 12, 2015

seangarner commented May 12, 2015

seangarner commented May 13, 2015

dougwilson commented May 13, 2015

seangarner commented May 13, 2015

dougwilson commented May 13, 2015

dougwilson May 13, 2015

seangarner May 13, 2015

dougwilson commented May 13, 2015

seangarner commented May 14, 2015

seangarner commented May 14, 2015

seangarner commented May 15, 2015

seangarner commented May 21, 2015

seangarner commented Jun 22, 2015

dougwilson commented Jul 2, 2015

dougwilson commented Jul 2, 2015

dougwilson commented Jul 2, 2015

dougwilson Jul 2, 2015

seangarner Jul 4, 2015

seangarner commented Jul 2, 2015

seangarner commented Jul 2, 2015

dougwilson Jul 10, 2015

switch user back to pool default on connection release #1088

switch user back to pool default on connection release #1088

Conversation

seangarner commented May 12, 2015

seangarner commented May 12, 2015

seangarner commented May 13, 2015

dougwilson commented May 13, 2015

seangarner commented May 13, 2015

dougwilson commented May 13, 2015

dougwilson May 13, 2015

Choose a reason for hiding this comment

seangarner May 13, 2015

Choose a reason for hiding this comment

dougwilson commented May 13, 2015

seangarner commented May 14, 2015

seangarner commented May 14, 2015

seangarner commented May 15, 2015

seangarner commented May 21, 2015

seangarner commented Jun 22, 2015

dougwilson commented Jul 2, 2015

dougwilson commented Jul 2, 2015

dougwilson commented Jul 2, 2015

dougwilson Jul 2, 2015

Choose a reason for hiding this comment

seangarner Jul 4, 2015

Choose a reason for hiding this comment

seangarner commented Jul 2, 2015

seangarner commented Jul 2, 2015

dougwilson Jul 10, 2015

Choose a reason for hiding this comment