Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes #1833 on postgres #2017

Merged
merged 9 commits into from Jun 1, 2017
Merged

fixes #1833 on postgres #2017

merged 9 commits into from Jun 1, 2017

Conversation

@smulesoft
Copy link
Contributor

@smulesoft smulesoft commented Apr 18, 2017

  • Tries to fix #1833 listening on event end on the connection
  • Adds integration test to reproduce the bug
@smulesoft
Copy link
Contributor Author

@smulesoft smulesoft commented Apr 18, 2017

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.
Copy link
Member

@elhigu elhigu left a comment

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?

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 comment has been minimized.

@elhigu

elhigu Apr 18, 2017
Member

This will collide with other postgresql servers.

This comment has been minimized.

@smulesoft

smulesoft Apr 19, 2017
Author Contributor

You think the test should run for every postgres version?

This comment has been minimized.

@elhigu

elhigu Apr 19, 2017
Member

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.

This comment has been minimized.

@smulesoft

smulesoft Apr 19, 2017
Author Contributor

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.

const $Docker = require('dockerode');

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

This comment has been minimized.

@elhigu

elhigu Apr 18, 2017
Member

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

This comment has been minimized.

@smulesoft

smulesoft Apr 19, 2017
Author Contributor

Yeap, you're right

@@ -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);

This comment has been minimized.

@elhigu

elhigu Apr 18, 2017
Member

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.

This comment has been minimized.

@smulesoft

smulesoft Apr 19, 2017
Author Contributor

Gone

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

require('./reconnect')(knex)

This comment has been minimized.

@elhigu

elhigu Apr 18, 2017
Member

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)

This comment has been minimized.

@smulesoft

smulesoft Apr 19, 2017
Author Contributor

Gone

@smulesoft
Copy link
Contributor Author

@smulesoft smulesoft commented Apr 19, 2017

@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 #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.

@smulesoft smulesoft force-pushed the smulesoft:bugs/gh-1833-reconnect branch from 2830a55 to 93436f6 Apr 19, 2017
@elhigu
Copy link
Member

@elhigu elhigu commented Apr 19, 2017

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

@smulesoft
Copy link
Contributor Author

@smulesoft smulesoft commented Apr 20, 2017

@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 smulesoft force-pushed the smulesoft:bugs/gh-1833-reconnect branch from 1a3ef17 to 9436c8f Apr 20, 2017
@smulesoft
Copy link
Contributor Author

@smulesoft smulesoft commented Apr 20, 2017

@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 smulesoft force-pushed the smulesoft:bugs/gh-1833-reconnect branch from 9436c8f to a71bccf Apr 20, 2017
@smulesoft
Copy link
Contributor Author

@smulesoft smulesoft commented May 8, 2017

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

@elhigu
Copy link
Member

@elhigu elhigu commented May 8, 2017

@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 smulesoft force-pushed the smulesoft:bugs/gh-1833-reconnect branch from 467fb31 to 6b270d9 May 15, 2017
@smulesoft
Copy link
Contributor Author

@smulesoft smulesoft commented May 15, 2017

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

1 similar comment
@smulesoft
Copy link
Contributor Author

@smulesoft smulesoft commented May 15, 2017

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

@elhigu
Copy link
Member

@elhigu elhigu commented May 15, 2017

@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
Copy link
Contributor Author

@smulesoft smulesoft commented May 15, 2017

@elhigu there!

@smulesoft smulesoft force-pushed the smulesoft:bugs/gh-1833-reconnect branch from 7f844c0 to 63b2952 May 16, 2017
@smulesoft
Copy link
Contributor Author

@smulesoft smulesoft commented May 16, 2017

@elhigu I can't find the error man

@smulesoft smulesoft force-pushed the smulesoft:bugs/gh-1833-reconnect branch 4 times, most recently from d1d5e33 to d28e9ca May 16, 2017
});
}

function waitReadyForQueries(attempt = 0) {

This comment has been minimized.

@smulesoft

smulesoft May 16, 2017
Author Contributor

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

@@ -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",

This comment has been minimized.

@elhigu

elhigu May 16, 2017
Member

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

This comment has been minimized.

@smulesoft

smulesoft May 16, 2017
Author Contributor

Oops, my bad

@smulesoft smulesoft force-pushed the smulesoft:bugs/gh-1833-reconnect branch 3 times, most recently from 3098b86 to c7e9148 May 16, 2017
@smulesoft
Copy link
Contributor Author

@smulesoft smulesoft commented May 16, 2017

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

@elhigu
Copy link
Member

@elhigu elhigu commented May 17, 2017

@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
Copy link
Contributor Author

@smulesoft smulesoft commented May 17, 2017

@elhigu 🎉

@elhigu
Copy link
Member

@elhigu elhigu commented May 18, 2017

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

@smulesoft smulesoft closed this May 18, 2017
@smulesoft smulesoft reopened this May 18, 2017
@elhigu
Copy link
Member

@elhigu elhigu commented May 21, 2017

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
Copy link
Contributor Author

@smulesoft smulesoft commented May 23, 2017

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

@cbartondock
Copy link

@cbartondock cbartondock commented May 27, 2017

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 elhigu force-pushed the smulesoft:bugs/gh-1833-reconnect branch from 233bad9 to 0aa5405 Jun 1, 2017
@elhigu
Copy link
Member

@elhigu elhigu commented Jun 1, 2017

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

@elhigu elhigu merged commit 31ba04a into knex:master Jun 1, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@elhigu
Copy link
Member

@elhigu elhigu commented Jun 1, 2017

Done, thanks @smulesoft for this!

@smulesoft
Copy link
Contributor Author

@smulesoft smulesoft commented Jun 1, 2017

No, no, thank you my man! Cheers

@damirm
Copy link

@damirm damirm commented Jun 1, 2017

You guys are awesome 👍

@smulesoft
Copy link
Contributor Author

@smulesoft smulesoft commented Jun 5, 2017

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

@davidgwking
Copy link

@davidgwking davidgwking commented Jun 7, 2017

@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
Copy link
Member

@elhigu elhigu commented Jun 7, 2017

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
Copy link

@davidgwking davidgwking commented Jul 24, 2017

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

@elhigu
Copy link
Member

@elhigu elhigu commented Jul 24, 2017

@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
Copy link

@lucaslencinas-zz lucaslencinas-zz commented Feb 23, 2018

Awesome @smulesoft alias Coco!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants