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

Support patched MSSQL lib #2861

Merged
merged 3 commits into from Nov 23, 2018

Conversation

Projects
None yet
3 participants
@dhensby
Copy link
Contributor

dhensby commented Oct 17, 2018

As discussed in previously (#2637 (comment)), the node-mssql lib pool depletion bug was patched in 4.2.1 (see tediousjs/node-mssql#683) and locking the version of mssql to 4.1.0 is counter productive.

This change aims to force either 4.1.0 (which is then monkey patched) or a version >= 4.2.1 <5 which is left alone.

Ideally this would make a 0.15.3 knex release, if possible.

I know the long-term goal is to drop the mssql dependency all together, but we quite need 4.2.1 and some of the patches in 0.15 of knex too, so this allows a intermediate resolution until mssql dependency is written out.

@dhensby

This comment has been minimized.

Copy link
Contributor

dhensby commented Oct 17, 2018

So test suite is failing because the code coverage has just dipped below the expected values, there doesn't appear to be an existing test for this feature, is there an appropriate way to test this?

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 17, 2018

I don't think that 0.15.3 is feasible as we don't have separate branch for 0.15 point releases and master went quite ahead since last 0.15 release with plenty of breaking changes.

Did you run all the stress tests on 4.2.1 for any meaningful amount of time to confirm that all the patched issues are actually resolved?

I think code coverage could be ignored for this one, it's not like we are going to test with different dependency versions.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 17, 2018

In order to avoid test failure, I would suggest to add istanbul ignore to that part of the code - https://github.com/gotwarlost/istanbul/blob/master/ignoring-code-for-coverage.md

@dhensby

This comment has been minimized.

Copy link
Contributor

dhensby commented Oct 17, 2018

I don't think that 0.15.3 is feasible as we don't have separate branch for 0.15 point releases and master went quite ahead since last 0.15 release with plenty of breaking changes.

OK - I understand if it's not practical... You could, in theory, create a hotfix branch off of the 0.15.2 tag, but I know it's not part of the usual release process.

Did you run all the stress tests on 4.2.1 for any meaningful amount of time to confirm that all the patched issues are actually resolved?

I have not, is there any guidance about how to do this?

I think code coverage could be ignored for this one, it's not like we are going to test with different dependency versions.

OK - I've added the appropriate ignore comments, let's hope it goes green this time

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 17, 2018

Thanks!
I don't think it's documented, take a look at "stress:init", "stress:test" and "stress:destroy" scripts in package.json, and I think @elhigu used to run it overnight to get meaningful results.

@dhensby dhensby force-pushed the dhensby:fix/mssql-support branch from a8e8a10 to 32d21b4 Oct 17, 2018

@dhensby

This comment has been minimized.

Copy link
Contributor

dhensby commented Oct 17, 2018

Hmm - interesting, the stress test is forcing mssql down to 0 queries, which is weird... maybe the connection depletion problem isn't completely resolved - I shall investigate

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 17, 2018

@dhensby Probably it would be useful to see what knex monkey patching is doing and see if it could be applied directly to node-mssql.

@dhensby

This comment has been minimized.

Copy link
Contributor

dhensby commented Oct 17, 2018

Yep

@dhensby

This comment has been minimized.

Copy link
Contributor

dhensby commented Oct 17, 2018

OK - I'm going to close this for now as it seems the mssql lib hasn't fixed the problem, it seems! I've pulled out just the fix and opened a PR here - tediousjs/node-mssql#719

Once that's in I'll revisit this.

@dhensby dhensby closed this Oct 17, 2018

@dhensby

This comment has been minimized.

Copy link
Contributor

dhensby commented Oct 17, 2018

Right, so it seems that this is actually a bug in tedious and not in node-mssql per se (well, it could be handled better in mssql) - see tediousjs/tedious#769 which was released in 2.7.1.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 17, 2018

@dhensby Did you try running stress test with an updated tedious version?

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Oct 18, 2018

@dhensby node-mssql 4.2.2 with fixed tedious was released, might make sense to give it a shot.

@dhensby dhensby reopened this Oct 18, 2018

@dhensby dhensby force-pushed the dhensby:fix/mssql-support branch from 32d21b4 to 4bee9c6 Oct 18, 2018

@dhensby

This comment has been minimized.

Copy link
Contributor

dhensby commented Oct 18, 2018

OK - bumped to 4.2.2 and it does fix the pool depletion bug this time

@dhensby

This comment has been minimized.

Copy link
Contributor

dhensby commented Oct 18, 2018

OK - this still fails stress testing now with a new error:

events.js:167
      throw er; // Unhandled 'error' event
      ^

Error: No event 'socketError' in state 'LoggedInSendingInitialSql'
    at Connection.dispatchEvent (/Users/dhensby/projects/dhensby/knex/node_modules/tedious/lib/connection.js:1014:28)
    at Connection.socketError (/Users/dhensby/projects/dhensby/knex/node_modules/tedious/lib/connection.js:1030:12)
    at Connection.socketEnd (/Users/dhensby/projects/dhensby/knex/node_modules/tedious/lib/connection.js:1047:14)
    at Socket.<anonymous> (/Users/dhensby/projects/dhensby/knex/node_modules/tedious/lib/connection.js:884:18)
    at Socket.emit (events.js:187:15)
    at endReadableNT (_stream_readable.js:1081:12)
    at process._tickCallback (internal/process/next_tick.js:63:19)
Emitted 'error' event at:
    at Connection.tedious.on.err (/Users/dhensby/projects/dhensby/knex/node_modules/mssql/lib/tedious.js:252:14)
    at Connection.emit (events.js:182:13)
    at Connection.dispatchEvent (/Users/dhensby/projects/dhensby/knex/node_modules/tedious/lib/connection.js:1014:14)
    at Connection.socketError (/Users/dhensby/projects/dhensby/knex/node_modules/tedious/lib/connection.js:1030:12)
    [... lines matching original stack trace ...]
    at process._tickCallback (internal/process/next_tick.js:63:19)

Edit: This may now be an issue in tedious itself :/

@dhensby dhensby closed this Oct 18, 2018

@dhensby dhensby reopened this Oct 18, 2018

@dhensby

This comment has been minimized.

Copy link
Contributor

dhensby commented Oct 18, 2018

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 18, 2018

@dhensby It would be fantastic to finish this in time before 0.16.0 release, it would really make life of all the Node.js/MSSQL users better. Is there anything I can do to help it happen?

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 18, 2018

Depends on node-mssql bumping tedious version to fixed one: tediousjs/node-mssql#733

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Nov 19, 2018

Also I need to check that this will still work as stable as patched version.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 20, 2018

@dhensby PR to bump tedious dependency raised: tediousjs/node-mssql#755

Show resolved Hide resolved src/dialects/mssql/index.js Outdated

@dhensby dhensby force-pushed the dhensby:fix/mssql-support branch from d936fe6 to 45fd21c Nov 22, 2018

dhensby added some commits Oct 17, 2018

@dhensby dhensby force-pushed the dhensby:fix/mssql-support branch from 45fd21c to 320a596 Nov 22, 2018

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 22, 2018

@dhensby Does it pass the stress test now?

@dhensby

This comment has been minimized.

Copy link
Contributor

dhensby commented Nov 22, 2018

YES! it passes, I no longer get the error detailed in #2861 (comment)

I also tested against 4.3.0 to make sure the tedious upgraded did fix it and it does (4.3.0 failed).

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 22, 2018

Woohoo!
@elhigu Would you like to check its stability from a branch or releasing within a "next" version for testing would be more convenient?

@dhensby

This comment has been minimized.

Copy link
Contributor

dhensby commented Nov 22, 2018

@kibertoad what I would say is that an alpha release is subject to breaking changes, so I don't know how wise it would be to merge this as-is.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 22, 2018

@dhensby It's a devDependency anyway, I don't imagine knex needing to change anything in production code after stable version is released.

@dhensby

This comment has been minimized.

Copy link
Contributor

dhensby commented Nov 22, 2018

ah, of course, good point

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Nov 23, 2018

Woohoo!
@elhigu Would you like to check its stability from a branch or releasing within a "next" version for testing would be more convenient?

It will be easier to run tests on this branch direclty. I'm on it.

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Nov 23, 2018

With fast testing this seems to be even more stable, than current master (I was able to crash that when socket is broken during login). 🎉 Next step would be to add deprecation message for that old mssql version and then in 0.17 we could remove that patching code altogether.

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Nov 23, 2018

I'll leave that stress test running during my lunch and then merge this if everything looks good.

Show resolved Hide resolved src/dialects/mssql/index.js Outdated
@elhigu

elhigu approved these changes Nov 23, 2018

Copy link
Collaborator

elhigu left a comment

Seems really stable 👍

@kibertoad kibertoad merged commit b56ff04 into tgriesser:master Nov 23, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.05%) to 84.941%
Details

@dhensby dhensby deleted the dhensby:fix/mssql-support branch Nov 23, 2018

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Nov 23, 2018

I suppose https://github.com/tgriesser/knex/blob/master/.travis.yml#L29 those allow failures travis builds should be still removed?

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 23, 2018

@elhigu You mean within the context of oracle fix PR, right?

@elhigu

This comment has been minimized.

Copy link
Collaborator

elhigu commented Nov 23, 2018

Yeah, I mixed up issues. In oracledb context indeed :)

mwilliammyers added a commit to voxjar/knex that referenced this pull request Dec 12, 2018

Support patched MSSQL lib (tgriesser#2861)
* PATCH Support patched MSSQL lib

* Add code coverage ignores to mssql monkey-patching

* Fix compatibility description
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment