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

Fix mssql driver crashing #2637

Merged
merged 3 commits into from Jun 10, 2018

Conversation

Projects
None yet
3 participants
@elhigu
Collaborator

elhigu commented May 30, 2018

Since mssql driver has not been maintained, locked driver version and monkey patched it to fix couple of problems of leaking connections / hanging and crashing when connections are closed unexpectedly.

We should eventually change mssql dialect to use directly tedious driver.

I did run this stress test script overnight and it didn't crash and memory consumption didn't grow either (it was stable between 150MB - 400MB)

while(true) {
await Bluebird.delay(20); // kill everything every quite often from server side
try {
await Promise.all([
killConnectionsPg(),
killConnectionsMyslq(mysql),
killConnectionsMyslq(mysql2),
// killConnectionsMyslq(mysql2),

This comment has been minimized.

@kibertoad

kibertoad May 30, 2018

Collaborator

Why was this commented out?

This comment has been minimized.

@elhigu

elhigu May 30, 2018

Collaborator

because mysql2 driver still crashes https://github.com/tgriesser/knex/pull/2637/files#diff-5463c94e7785e7603f6dfc48e598c428R162

If that driver is fixed, I can enable lines 164,165 and 178

},
pool: { max: 50 }
});
/* TODO: figure out how to nicely install oracledb node driver on osx
const mysql = Knex({
client: 'oracledb',

This comment has been minimized.

@kibertoad

kibertoad May 31, 2018

Collaborator

btw, why mysql is using oracledb client?..

This comment has been minimized.

@elhigu

elhigu May 31, 2018

Collaborator

just some initial copy-paste for writing oracledb test after mssql is all done.

@elhigu elhigu merged commit ae1245e into tgriesser:master Jun 10, 2018

1 check passed

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

This comment has been minimized.

dhensby commented Sep 11, 2018

Is this problem now fixed with the release of 4.2.1? (we got a PR merged that fixed the depletion of connections error - tediousjs/node-mssql#683)

@kibertoad

This comment has been minimized.

Collaborator

kibertoad commented Sep 11, 2018

@elhigu We can probably try updating driver, removing monkey-patching and running existing stress tests?..

@dhensby

This comment has been minimized.

dhensby commented Sep 11, 2018

It would be very helpful if this limitation was removed because we can't upgrade our knex version as we need the mssql fix in production ;)

@kibertoad

This comment has been minimized.

Collaborator

kibertoad commented Sep 11, 2018

@dhensby Doesn't monkey-patching introduced by @elhigu solve the same issue as well in newer knex version?

@dhensby

This comment has been minimized.

dhensby commented Sep 12, 2018

@kibertoad TBH, I'm not sure and it's not something we'd be prepared to put time into investigating. We had issues on azure in our production application which we spent a long time debugging and resolving, culminating in getting tediousjs/node-mssql#683 merged.

It's not going to make business sense for us to downgrade from a known working patch "just" so we can upgrade knex.

The reason to upgrade knex would be so that knex.schema.unique and knex.schema.dropUnique do as expected. The work around to that is far more simple and less risky than rolling back a working patch and risking depleting our connection pool in prod again.

For now we'll use knex.schema.unique([cols], 'our_own_index_name'); and knex.schema.dropIndex([cols], 'our_own_index_name'); which works and I'm sure you can appreciate is insignificant in terms of risk to prod vs downgrading the mssql lib.

@kibertoad

This comment has been minimized.

Collaborator

kibertoad commented Sep 12, 2018

@dhensby That makes sense. I'll try to get stress tests working locally on my machine to get the answer.

@kibertoad

This comment has been minimized.

Collaborator

kibertoad commented Sep 12, 2018

@dhensby Wait, was that version ever released? https://www.npmjs.com/package/mssql still shows 4.1.0 as the latest published one.

@dhensby

This comment has been minimized.

dhensby commented Sep 12, 2018

https://www.npmjs.com/package/mssql/v/4.2.1 - it seems to be @next for some reason, but we're definitely using it...

It's not the most maintained package out there ;)

@kibertoad

This comment has been minimized.

Collaborator

kibertoad commented Sep 12, 2018

@dhensby Does stability-wise (other than this specific issue) 4.2.1 perform same or better than the current "stable" 4.1.0 for you? In general it is preferable to avoid "next" dependencies in libraries, I wonder if we should make an exclusion for this particular case.

@dhensby

This comment has been minimized.

dhensby commented Sep 12, 2018

Yeah, that's a good point, I hadn't realised it was a "next" release, though, anyone wanting to run mssql needs the fix that is in 4.2.1.

In production we haven't had any issues with MSSQL, the library or other symptoms since upgrading to 4.2.1, so we are happy with its stability.

Another option would be to allow users to set some kind of config to opt-out of the 4.1.0 restriction so those that want to run the higher versions can?

This is the diff between 4.1.0 and 4.2.1 (tediousjs/node-mssql@85c14ff...7f374a8) and it doesn't look too risky to me.

@dhensby

This comment has been minimized.

dhensby commented Sep 12, 2018

I've asked that they release 4.2.1 as stable (tediousjs/node-mssql#703) though not sure how likely that is to be actioned :(

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Sep 12, 2018

I'd still rather get rid of mssql driver and use tedious directly than update knex to support newer mssql. It will be easier to make to work in a robust manner than trying to fix mssql to work well.

I'm open to help / review if anyone is going to change mssql -> tedious, but I really don't want to work with node-mssql's code anymore.

@kibertoad

This comment has been minimized.

Collaborator

kibertoad commented Sep 12, 2018

@elhigu I can absorb the effort of updating the supported mssql version as that doesn't sound like lots of work, but mssql -> tedious is going to require much more elaborate knowledge of SSQL and Tedious, and I don't even use MSSQL myself :D

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Sep 12, 2018

Not much more plain sql knowledge is needed... node-mssql already uses tedious pretty straightforward manner, but ot just forces to use one additional (and buggy) generic-pool for getting connections which causes most of the problems.

@kibertoad

This comment has been minimized.

Collaborator

kibertoad commented Sep 12, 2018

There is already a PR to replace generic-pool with Tarn.js for node-mssql. Would working on it to make it merged help alleviate the pain?

@dhensby

This comment has been minimized.

dhensby commented Sep 12, 2018

I'd still rather get rid of mssql driver and use tedious directly than update knex to support newer mssql. It will be easier to make to work in a robust manner than trying to fix mssql to work well.

Whilst I totally understand that and using the mssql package has caused us a lot of pain too, I don't think that larger objective should block derestricting the package currently supported.

For lots of developers (especially ones in our position) throwing out node-mssql is non-starter and simply means we won't be able to upgrade knex, which would be a shame

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Sep 12, 2018

There is already a PR to replace generic-pool with Tarn.js for node-mssql. Would working on it to make it merged help alleviate the pain?

I would rather see PR which allows to use node-mssql without any additional pool on top of tedious. Tarn is developed in terms of knex, so it might not be the best choice for mssql driver, but yeah with that it would be definitely easier to fix at least.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Sep 12, 2018

Whilst I totally understand that and using the mssql package has caused us a lot of pain too, I don't think that larger objective should block derestricting the package currently supported.

For lots of developers (especially ones in our position) throwing out node-mssql is non-starter and simply means we won't be able to upgrade knex, which would be a shame

How are you actually using mssql driver directly? What difference it makes for you if you have mssql wrapping tedious when you are actually using knex to interface it?

@dhensby

This comment has been minimized.

dhensby commented Sep 12, 2018

We only use knex for our DB migrations / schema building. All our DB interfacing happens through stored procedures and an internal DB SDK that provides an API to those stored procedures.

As knex doesn't support stored procedures we don't use knex for our actual DB SDK and so use the mssql package to bind the params and execute the SPs.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Sep 12, 2018

In that case you can still use knex the same way that before and node-mssql for the same stuff you are using it currently.

Knex just internally wont use node-mssql anymore.

@dhensby

This comment has been minimized.

dhensby commented Sep 12, 2018

That's a good point ;)

veikovx added a commit to veikovx/knex that referenced this pull request Sep 17, 2018

Fix mssql driver crashing (tgriesser#2637)
* Run SQL Server tests locally running SQL server in docker

* WIP mssql test stuff

* Patched MSSQL driver to not crash knex anymore

elhigu added a commit that referenced this pull request Sep 26, 2018

Kill queries after timeout for PostgreSQL (#2636)
* Kill queries after timeout for PostgreSQL

* Fix cancellation connection acquiring and test.

* Fix releasing connection in case query cancellation fails

* Add support for native enums on Postgres (#2632)

Reference https://www.postgresql.org/docs/current/static/sql-createtype.html

Closes #394

Signed-off-by: Will Soto <will.soto9@gmail.com>

* Removal of 'skim' (#2520)

* Allow overwriting log functions (#2625)

* Example build of custom log functions
* Handle logger object better for transactions
* Adjust test to ignore sqlite warning message

* Fixed onIn with empty values array (#2513)

* Drop support for strong-oracle (#2487)

* Remove babel-plugin-lodash (#2634)

While in theory, this may reduce the bundle size,
in practice it adds a ton of overhead during startup
due to the number of additional requires. Bundle
size also shouldn't matter for server side modules.

* Add json support to the query builder for mysql (#2635)

* Add json support to the query builder for mysql

refs #1036

Based on #1902

* Clarify supported version

* fix wrapIdentifier not being called in postgres alter column (#2612)

* fix wrapIdentifier not being called in postgres alter column

* add test for wrapIdentifier call in postgres alter column

* add comment regarding issue

* add issue & PR #'s in comment

* Remove readable-stream and safe-buffer (#2640)

* Add json support to the query builder for mysql

refs #1036

Based on #1902

* Clarify supported version

* Use native Buffer and Stream implementations

* fixes 2630 (#2642)

* Timeout errors shouldn't silently ignore the passed errors, but rather reject with original error. Fixes #2582 (#2626)

* chore: add Node.js 10 (#2594)

* chore: add Node.js 10

* chore: trigger new build

* chore: update lockfile

* chore: trigger new build

* fix: use npm i instead of npm ci

* Fix mssql driver crashing (#2637)

* Run SQL Server tests locally running SQL server in docker

* WIP mssql test stuff

* Patched MSSQL driver to not crash knex anymore

* Removed semicolon from rollback stmt for oracle (#2564)

* Remove WebSQL dialect (#2647)

* Add json support to the query builder for mysql

refs #1036

Based on #1902

* Clarify supported version

* Use native Buffer and Stream implementations

* Remove WebSQL dialect

* add homepage field to package.json (#2650)

* Make the stream catch errors in the query (#2638)

* Make the stream catch errors in the query

* Fix another case in which stream doesnt emits error

* Linter stuff

* Remove setTimeout in tests

* Make a test not to check the MySQL error code

* Fix stream error catching for MariaDB and PostgreSQL

* Fix stream error catching in Oracle

* Throw the error after emitting it to the stream

* Throw the error without instantiating a new Error

* Various fixes to mssql dialect (#2653)

* Fixed float type of mssql to be float

* Many tests where postgres test was not actually ran at all

* Migrations to be mssql compatible

Mssql driver doesn't handle if multiple queries are sent to same transaction concurrently.

* Prevented mssql failing when invalid schema builder was executed by accident

Instead of trying to generate sql from broken schema calls, just make exception to leak before query compiling is started

* Fixed mssql trx rollback to always throw an error

Also modified some connection test query to be mssql compatible

* Fixed various bugs from MSSQL driver to make tests run

* Fixed mssql unique index to be compatible with other dialect implementations

* Enable running mssql tests on CI

* Test for #2588

* Updated tests to not be dependend on tables left from previous test rans

* Trying to make mssql server work on travis

* Updated changelog and version

* Drop mariadb support  (#2681)

* Dropped support for mariasql

* ESLint

* Fixed docs to build again

* Fix knex.initialize() and adds test (#2477)

* Fix knex.initialize() and adds test

* Fix knex.initialize() and adds test

* Added test for reinitializing pool after destroy

* Fixed destroy / reinitialization test

* Fixed the fix

* Implement missing schema support for mssql dialect

* chore: Update dependencies. Remove estraverse (#2691)

* Update dependencies. Remove estraverse

* Fix compatibility with new Sinon

* Increase mssql timeout

* Normalized and validated driverNames of test db clients and fixed oracle test setup (#2692)

* Normalized and validated driverNames of test db clients and fixed oracle test setup

* Fixed failed queries from old query building tests which hadn't been ran in ages

* Allow selecting node version which is used to run oracledb docker tests

* Improved sql tester error messages

* Fixed rest of the oracledb tests

* Removed invalid flag from docker-compose

* Print mssql logs if initialization fails

* Fixed syntax error + final tests

* Added restart of failure for mssql DB initialization to try again if server was not ready

* Printout always mssql logs after container is started

* Fixed wait time printing after trying to connect

* Use npm run oracledb:test for testing oracle in travis

* Add Prettier (#2697)

* Add prettier
* Run files through prettier

* Istanbul -> NYC (#2700)

* istanbul -> nyc

* Update mocha

* Enforce code coverage (#2702)

* Enforce code coverage
* Update coverage numbers with current values

* version assignment on base class, copy on tx client, fix #2705

* Update changelog

* v0.15.1

* Added build step to test script. Fixes #2541 (#2712)

* Revert "Added build step to test script. Fixes #2541 (#2712)" (#2714)

This reverts commit 90ed8db.

* Allow oracle failures for now

* Fix issue with select(0) (#2711)

* Fix issue with select(0). Fixes #2658

* Refactor migrator (#2695)

* Refactor migrator

* Fix exports

* Fix ESLint

* Fix migrator

* Fix reference to config

* Split some more

* Fix table builder

* Fix argument order

* Merge branch 'master' of https://github.com/tgriesser/knex into feature/2690-support-multiple-migration-dirs

# Conflicts:
#	src/migrate/index.js
#	test/index.js
#	test/unit/migrate/migrator.js

* Fix #2715 (#2721)

* Fix #2715, explicitly set precision in datetime & timestamp
* Allow for precision in knex.fn.now, mysql time
* Add test for datetime with precision

* Bump changelog

* 0.15.2

* Fix issues with warnPromise when migration does not return a promise. Fixes #2725 (#2730)

* Add tests for multiple union arguments with callbacks and builders (#2749)

* Add tests for multiple union arguments

* Add some callback and raw tests to union unit tests

* Compile with before update so that bindings are put in correct order (#2733)

* Fix join using builder withSchema. (#2744)

* Use Datetime2 for MSSQL datetime + timestamp types (#2757)

* Use Datetime2 for MSSQL datetime + timestamp types

Datetime2 is now the recommended column type for new date work: https://docs.microsoft.com/en-us/sql/t-sql/data-types/datetime-transact-sql?view=sql-server-2017

* Add tests for MSSQL datetime2 changes

* General/document breaking change (#2774)

* Add changelog entry for a breaking change

* Improve entry

* Allow timestamp with timezone on mssql databases (#2724)

* Allow timestamp with timezone on mssql databases

* Change timestamp parameter to use object instead of boolean

* Update dependencies (#2772)

* Feature/2690: Multiple migration directories (#2735)

* Implement support for multiple migration directories

* Add tests for new functionality

* Fix tape tests

* Pass migration directory upwards

* Fix multiple directory support

* Fix bugs

* Rollback correctly

* Fix remaining tests

* Address comments

* #2758: Implement fail-fast logic for dialect resolution (#2776)

* Implement fail-fast logic for dialect resolution, clean-up code around.

* Remove method that was deprecated long time ago

* Address additional comments

* Try addressing comments

* Set client explicitly

* Fix compatibility with older Node versions

* Use columnize instead of wrap in using(). (#2713)

* Use columnize instead of wrap in using().

This is an attempt to fix #2136. Also added an integration test, couldn't find any existing.

* Exclude MSSQL from test as it does not support .using

* Change test to not use subquery

* Change test

* Introduced abstraction for getting migrations (#2775)

* Introduced abstraction for getting migrations

This would allow a webpack migration source which is compatible with bundling.

* Fixed migration validation with custom migration source

* Fixed issues after rebasing on muti directory PR

* Renamed parameter and fixed error message

* Addressed some PR comments

* Finished comment

* Moved filename extension filtering into fs-migrator

* Added test showing in memory custom migration source

* Cleaned up how to get config

* Fixed failing test

* Hopefully fix tests

* Fix Node.js 10 support in tests

* Test for correctly releasing cancel query connection
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment