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

Conversation

Projects
None yet
6 participants
@smulesoft
Contributor

smulesoft commented Apr 18, 2017

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

This comment has been minimized.

Contributor

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

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

Collaborator

This will collide with other postgresql servers.

This comment has been minimized.

@smulesoft

smulesoft Apr 19, 2017

Contributor

You think the test should run for every postgres version?

This comment has been minimized.

@elhigu

elhigu Apr 19, 2017

Collaborator

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

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

Collaborator

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

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

Collaborator

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

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

Collaborator

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

Contributor

Gone

@smulesoft

This comment has been minimized.

Contributor

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.

@elhigu

This comment has been minimized.

Collaborator

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

This comment has been minimized.

Contributor

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

This comment has been minimized.

Contributor

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

This comment has been minimized.

Contributor

smulesoft commented May 8, 2017

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

@elhigu

This comment has been minimized.

Collaborator

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

This comment has been minimized.

Contributor

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

This comment has been minimized.

Contributor

smulesoft commented May 15, 2017

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

@elhigu

This comment has been minimized.

Collaborator

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

This comment has been minimized.

Contributor

smulesoft commented May 15, 2017

@elhigu there!

@elhigu

This comment has been minimized.

Collaborator

elhigu commented May 16, 2017

@smulesoft

This comment has been minimized.

Contributor

smulesoft commented May 16, 2017

@elhigu I can't find the error man

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

This comment has been minimized.

This comment has been minimized.

@smulesoft

smulesoft May 16, 2017

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

Collaborator

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

Contributor

Oops, my bad

@smulesoft

This comment has been minimized.

Contributor

smulesoft commented May 16, 2017

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

@elhigu

This comment has been minimized.

Collaborator

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

This comment has been minimized.

Contributor

smulesoft commented May 17, 2017

@elhigu 🎉

@elhigu

This comment has been minimized.

Collaborator

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

This comment has been minimized.

Collaborator

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

This comment has been minimized.

Contributor

smulesoft commented May 23, 2017

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

@cbartondock

This comment has been minimized.

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

This comment has been minimized.

Collaborator

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 tgriesser:master Jun 1, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@elhigu

This comment has been minimized.

Collaborator

elhigu commented Jun 1, 2017

Done, thanks @smulesoft for this!

@smulesoft

This comment has been minimized.

Contributor

smulesoft commented Jun 1, 2017

No, no, thank you my man! Cheers

@damirm

This comment has been minimized.

damirm commented Jun 1, 2017

You guys are awesome 👍

@smulesoft

This comment has been minimized.

Contributor

smulesoft commented Jun 5, 2017

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

@davidgwking

This comment has been minimized.

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

This comment has been minimized.

Collaborator

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

This comment has been minimized.

davidgwking commented Jul 24, 2017

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

@elhigu

This comment has been minimized.

Collaborator

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

This comment has been minimized.

lucaslencinas 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