fixes #1833 on postgres #2017

smulesoft · 2017-04-18T13:29:26Z

Tries to fix Reconnecting after database server disconnect/reconnect #1833 listening on event end on the connection
Adds integration test to reproduce the bug

smulesoft · 2017-04-18T13:32:20Z

You can see that:

If you undo the change on client.js the test will either sometimes fail or hang.
With the changes on client.js the test always passes. Sometimes though, the afterEach hangs trying to destroy the connection-pool. I'm not sure what to do on that case (removing the destroy() call seems to work.)
I commented out the tests for other SQLs, these should be reactivated.

elhigu

Great work! I left some initial comments... Even that I hate to take docker in here I must admit that I cannot thing any better way either to test this one.

Also this test could be added also to mysql it would be awesome.

About the current status of the code I would be more confident about this if we would know the root cause why knex.destroy hangs some times and why test doesn't fail some times even without the change. Maybe printing out data from pool's internal state could help to figure that out?

elhigu · 2017-04-18T19:03:27Z

test/integration/reconnect/postgresContainer.js

+  var image    = _.get(options, 'image', 'postgres:9.6');
+  var username = _.get(options, 'username', 'postgres');
+  var password = _.get(options, 'password', '');
+  var hostPort = _.get(options, 'hostPort', 5432);


This will collide with other postgresql servers.

You think the test should run for every postgres version?

No. I mean that usually CI machine (and pretty much every developers machine) is already running the default postgres server in that port. So if you are trying to use the same port for postgres which is ran in docker, starting the server should fail.

Oh, OK. 5432 was just the default value. The docker container was actually running on hostPort: 49152. You can this configuration now on knexfile.js.

elhigu · 2017-04-18T19:03:32Z

test/integration/reconnect/docker.js

+const $Docker = require('dockerode');
+
+function Docker() {
+  this.dockerAPI = new $Docker({ socketPath: '/var/run/docker.sock' });


This isn't probably windows compatible. Is there way to use some other communication method that is not unix domain socket?

Yeap, you're right

elhigu · 2017-04-18T19:05:27Z

test/knexfile.js

@@ -6,7 +6,7 @@ var _          = require('lodash');
 var Promise    = require('bluebird');

 // excluding oracle and mssql dialects from default integrations test
-var testIntegrationDialects = (process.env.DB || "maria mysql mysql2 postgres sqlite3").match(/\w+/g);
+var testIntegrationDialects = (process.env.DB || "postgres" || "maria mysql mysql2 postgres sqlite3").match(/\w+/g);


Why is this here? locally you can use DB env variable to select which bds are tested. IIRC they are set in .travis configuration for knex CI.

elhigu · 2017-04-18T19:07:00Z

test/integration/suite.js

@@ -14,6 +14,7 @@ module.exports = function(knex) {
      return knex.destroy()
    })

+    require('./reconnect')(knex)


should probably not use the same connection setup that is used for other integration tests, because of many reasons (one is that server must be in different port e.g. 15432). If there is a way to check that is docer host or something is available and only in that case require this test and make it completely separated (it can contain its own connection settings inside the test case)

smulesoft · 2017-04-19T15:16:47Z

@elhigu I made corrections:

I only run the tests when the machine platform is linux and has docker installed (see cancanRunDockerTests in test/docker/index.js)
I refactored the test so we can run this test on mysql too. See the changes in knexfile.js and test/docker/index.js.
Because the test runs OK in mysql without the change on client.js, I moved the change to the postgres connector (exactly as @damirm commented in Reconnecting after database server disconnect/reconnect #1833)

Please check and read the code, I don't have much more time to work on this, I'm busy with a load of stuff.

elhigu · 2017-04-19T15:22:19Z

@smulesoft thanks, I'll check this out when I get some spare time (I have to test this stuff out on windows too).

smulesoft · 2017-04-20T12:16:49Z

@elhigu

I added a new test: checking that when the db-container is stopped any attempt to make queries fails (just to make sure.)

Just so you know, I am running the tests with these commands:

$ npm run babel; DB=postgres npm run test
$ npm run babel; DB=mysql npm run test

To run tests you should have docker installed (and I'm not sure if you should also have the images postgres:9.6 and mysql:5.7 installed as well.)

smulesoft · 2017-04-20T13:12:45Z

@elhigu Regarding why sometimes the destroy() call on afterEach hook fails:

When this happens, you can see in the logs there is an error on calling Client_PG.checkVersion:

Unhandled rejection Error: Connection terminated
    at Connection.<anonymous> (/stmp/knex/node_modules/pg/lib/client.js:199:29)
    at Connection.g (events.js:291:16)
    at emitNone (events.js:86:13)
    at Connection.emit (events.js:185:7)
    at Socket.<anonymous> (/stmp/knex/node_modules/pg/lib/connection.js:141:10)
    at emitNone (events.js:91:20)
    at Socket.emit (events.js:185:7)
    at endReadableNT (_stream_readable.js:974:12)
    at _combinedTickCallback (internal/process/next_tick.js:80:11)
    at process._tickCallback (internal/process/next_tick.js:104:9)
From previous event:
    at Client_PG.checkVersion (/stmp/knex/lib/dialects/postgres/index.js:9:9903)
    at /stmp/knex/lib/dialects/postgres/index.js:9:9116
    at Connection.<anonymous> (/stmp/knex/node_modules/pg/lib/client.js:158:7)
    at Connection.g (events.js:291:16)
    at emitOne (events.js:101:20)
    at Connection.emit (events.js:188:7)
    at Socket.<anonymous> (/stmp/knex/node_modules/pg/lib/connection.js:136:12)
    at emitOne (events.js:96:13)
    at Socket.emit (events.js:188:7)
    at readableAddChunk (_stream_readable.js:176:18)
    at Socket.Readable.push (_stream_readable.js:134:10)
    at TCP.onread (net.js:551:20)

Or

Unhandled rejection error: terminating connection due to unexpected postmaster exit
    at Connection.parseE (/stmp/knex/node_modules/pg/lib/connection.js:569:11)
    at Connection.parseMessage (/stmp/knex/node_modules/pg/lib/connection.js:396:17)
    at Socket.<anonymous> (/stmp/knex/node_modules/pg/lib/connection.js:132:22)
From previous event:
    at Client_PG.checkVersion (/stmp/knex/lib/dialects/postgres/index.js:9:9903)
    at /stmp/knex/lib/dialects/postgres/index.js:9:9116
    at Connection.<anonymous> (/stmp/knex/node_modules/pg/lib/client.js:158:7)
    at Connection.g (events.js:291:16)
    at emitOne (events.js:101:20)
    at Connection.emit (events.js:188:7)
    at Socket.<anonymous> (/stmp/knex/node_modules/pg/lib/connection.js:136:12)
    at emitOne (events.js:96:13)
    at Socket.emit (events.js:188:7)
    at readableAddChunk (_stream_readable.js:176:18)
    at Socket.Readable.push (_stream_readable.js:134:10)
    at TCP.onread (net.js:551:20)

smulesoft · 2017-05-08T11:40:32Z

@elhigu Can we speed this up? Or, at least, can we merge the solution w/o the tests?

elhigu · 2017-05-08T12:34:29Z

@smulesoft I'm a bit reluctant to take anything in without tests, because if tests are not part of the same pull request it effectively means that those test will never be added. What could be done to make these tests work?

smulesoft · 2017-05-15T15:22:12Z

@elhigu Tests are passing! You can check again this PR and let me know what you think

smulesoft · 2017-05-15T15:27:30Z

@elhigu Tests are passing! You can check again this PR and let me know what you think

elhigu · 2017-05-15T16:59:10Z

@smulesoft thanks for updates, it seems to be almost working. There are still couple of strange problems:

Arrow functions doesn't work on old node:
https://travis-ci.org/tgriesser/knex/jobs/232479384#L2448

Weird syntax error:
https://travis-ci.org/tgriesser/knex/jobs/232479385#L1156
https://travis-ci.org/tgriesser/knex/jobs/232479386#L1516

Any idea why these happen? node 6 and 7 tests seems to work correctly though 👍

smulesoft · 2017-05-15T19:28:46Z

@elhigu there!

elhigu · 2017-05-16T10:24:09Z

@smulesoft still template strings are failing on 0.12
https://travis-ci.org/tgriesser/knex/jobs/232557372#L2453

Unexpected token = with node 4 & 5
https://travis-ci.org/tgriesser/knex/jobs/232557373#L1156
https://travis-ci.org/tgriesser/knex/jobs/232557374#L1516

Node 6 & 7 works

smulesoft · 2017-05-16T13:15:05Z

@elhigu I can't find the error man

elhigu · 2017-05-16T15:09:06Z

test/docker/reconnect.js

+    });
+  }
+
+  function waitReadyForQueries(attempt = 0) {


here https://travis-ci.org/tgriesser/knex/jobs/232836998#L2446

Arghh thanks man!
Any reason why you don't transpile test sources?

elhigu · 2017-05-16T15:12:16Z

package.json

@@ -72,9 +73,11 @@
    "lint": "eslint src/**",
    "plaintest": "mocha --check-leaks -b -R spec test/index.js && npm run tape",
    "prepublish": "npm run babel",
+    "docker_test": "bash ./scripts/docker-for-test.js",


I just noticed that this shell script is actually .js, should be .sh I'll try this thing on windows and check if it works correctly

Oops, my bad

smulesoft · 2017-05-16T19:29:07Z

@elhigu How can I fix all the syntax errors with a local command?

elhigu · 2017-05-17T07:27:58Z

@smulesoft you should see all the errors if you install node 0.12 with (pretty easy with nvm / docker) and running tests with it. Now there was destructuring used here

https://travis-ci.org/tgriesser/knex/jobs/232875884#L2446

smulesoft · 2017-05-17T16:50:29Z

@elhigu 🎉

elhigu · 2017-05-18T11:45:12Z

Great and looks good. I'll still need to run this on windows to make sure we are not breaking test runner there.

elhigu · 2017-05-21T21:09:49Z

I just did run this on windows with git bash and cmd, both were failing due to some unix specific stuff like /dev/null file doesnt exist. I'll try to fix windows compatibility soonish (I'm changing house during next weeks so it might take a while).

smulesoft · 2017-05-23T12:59:05Z

@elhigu Alright, good luck with everything! I'll keep an eye on this for more news

cbartondock · 2017-05-27T07:53:17Z

Can't wait to see this merged - I've been tracking issue #1833 for a while and I thought you guys had given up. I've only just now found the cutting edge - good luck!

elhigu · 2017-06-01T19:06:06Z

I just fixed the windows compatibility problem and rebased to get one failing tests to start working... hope that travis still approves this.

elhigu · 2017-06-01T19:41:58Z

Done, thanks @smulesoft for this!

smulesoft · 2017-06-01T19:51:30Z

No, no, thank you my man! Cheers

damirm · 2017-06-01T20:37:09Z

You guys are awesome 👍

smulesoft · 2017-06-05T14:35:56Z

@elhigu When do you think you are having your next release?

davidgwking · 2017-06-07T03:37:33Z

@smulesoft thank you for this fix! knex now behaves more or less how I'd expect! 🥇

would be great to see this released asap

elhigu · 2017-06-07T17:53:59Z

Sorry for being unresponsive. Still really busy with changing apartments (and doing some upgrade to old one). I cannot promise that I'm able to release this month.

davidgwking · 2017-07-24T18:48:34Z

@elhigu any update on when we could expect a release?

elhigu · 2017-07-24T18:52:16Z

@davidgwking I still don't want to make any promises that I might not be able to keep... my busyness is slowing down bit by bit though.

lucaslencinas-zz · 2018-02-23T19:18:17Z

Awesome @smulesoft alias Coco!!

smulesoft mentioned this pull request Apr 18, 2017

Reconnecting after database server disconnect/reconnect #1833

Closed

elhigu reviewed Apr 18, 2017

View reviewed changes

smulesoft force-pushed the bugs/gh-1833-reconnect branch from 2830a55 to 93436f6 Compare April 19, 2017 15:20

smulesoft force-pushed the bugs/gh-1833-reconnect branch from 1a3ef17 to 9436c8f Compare April 20, 2017 13:04

smulesoft force-pushed the bugs/gh-1833-reconnect branch from 9436c8f to a71bccf Compare April 20, 2017 13:18

smulesoft force-pushed the bugs/gh-1833-reconnect branch from 467fb31 to 6b270d9 Compare May 15, 2017 14:55

smulesoft force-pushed the bugs/gh-1833-reconnect branch from 7f844c0 to 63b2952 Compare May 16, 2017 13:07

smulesoft force-pushed the bugs/gh-1833-reconnect branch 4 times, most recently from d1d5e33 to d28e9ca Compare May 16, 2017 14:04

elhigu reviewed May 16, 2017

View reviewed changes

smulesoft force-pushed the bugs/gh-1833-reconnect branch 3 times, most recently from 3098b86 to c7e9148 Compare May 16, 2017 15:35

smulesoft closed this May 18, 2017

smulesoft reopened this May 18, 2017

smulesoft and others added 9 commits June 1, 2017 22:01

fixes knex#1833 on postgres

5a81140

adds @elhigu requested changes

2e4fbfd

adds test when database is down

a9c5000

adds script for docker support

60240c9

removes old comment

4e7ec0d

fixes syntax

837199d

fixes more syntax errors

8e8ea55

fixes final syntax errors

a2df0a3

Docker testing enabled only on linux for now

0aa5405

elhigu force-pushed the bugs/gh-1833-reconnect branch from 233bad9 to 0aa5405 Compare June 1, 2017 19:05

elhigu merged commit 31ba04a into knex:master Jun 1, 2017

fixes #1833 on postgres #2017

fixes #1833 on postgres #2017

Conversation

smulesoft commented Apr 18, 2017

smulesoft commented Apr 18, 2017 • edited Loading

elhigu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smulesoft commented Apr 19, 2017

elhigu commented Apr 19, 2017 • edited Loading

smulesoft commented Apr 20, 2017

smulesoft commented Apr 20, 2017

smulesoft commented May 8, 2017

elhigu commented May 8, 2017

smulesoft commented May 15, 2017

smulesoft commented May 15, 2017

elhigu commented May 15, 2017

smulesoft commented May 15, 2017

elhigu commented May 16, 2017

smulesoft commented May 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smulesoft commented May 16, 2017

elhigu commented May 17, 2017

smulesoft commented May 17, 2017

elhigu commented May 18, 2017

elhigu commented May 21, 2017

smulesoft commented May 23, 2017 • edited Loading

cbartondock commented May 27, 2017

elhigu commented Jun 1, 2017

elhigu commented Jun 1, 2017

smulesoft commented Jun 1, 2017

damirm commented Jun 1, 2017

smulesoft commented Jun 5, 2017

davidgwking commented Jun 7, 2017

elhigu commented Jun 7, 2017

davidgwking commented Jul 24, 2017

elhigu commented Jul 24, 2017

lucaslencinas-zz commented Feb 23, 2018

smulesoft commented Apr 18, 2017 •

edited

Loading

elhigu commented Apr 19, 2017 •

edited

Loading

smulesoft commented May 23, 2017 •

edited

Loading