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

chore(build): enable mssql on travis #3327

Merged
merged 1 commit into from
Feb 9, 2019
Merged

chore(build): enable mssql on travis #3327

merged 1 commit into from
Feb 9, 2019

Conversation

vlapo
Copy link
Contributor

@vlapo vlapo commented Dec 25, 2018

Lets try travis with mssql. I want to see travis build and try rerun few times if it is stable.

@amaranth
Copy link
Contributor

amaranth commented Jan 3, 2019

It looks like this passed but something went weird with the test run on node 10 and it timed out.

@vlapo
Copy link
Contributor Author

vlapo commented Jan 3, 2019

Yeah, there is issue with mssql driver. Issue is reported and we are waiting to resolve. Will see in few days :-)

@pleerock
Copy link
Member

pleerock commented Jan 3, 2019

@vlapo can you please provide more info - where issue is reported? btw, today I added CircleCI config, so we can also try to setup it as well.

@vlapo
Copy link
Contributor Author

vlapo commented Jan 3, 2019

I think there is problem with unclosed connections in pool. Issue in mssql driver tediousjs/node-mssql#457 and in node-pool library coopernurse/node-pool#159.

The latest mocha do not exit properly if there are unclosed connections or resources. There is simple workaround - adding --exit flag for now but I hope issue will be resolved soon.

Note: Just do not know why is this issue only with node v10.

@dhensby
Copy link
Contributor

dhensby commented Jan 24, 2019

  1. You need to close your connection pools
  2. You can use the --exit flag if you want the old mocha behaviour of terminating processes once all the test callbacks are complete.

@vlapo
Copy link
Contributor Author

vlapo commented Jan 24, 2019

  1. Yeah I see problem in typeorm code. We do not wait until connection pool is closed in SqlDriver. But I fixed this locally and have same issue with mocha.
  2. Sure we know about this possibility but its just a workaround not solution of problem :)

I still thinks that problem is in node-pool library. Lets wait to fix and see if it will be help.

@dhensby
Copy link
Contributor

dhensby commented Jan 24, 2019

@vlapo are you able to track down what connections are left open that are preventing the process from exiting?

I found for my test suites that maintaining a good record of connections and closing them was the solution, I didn't find any underlying issue with node-pool

@vlapo
Copy link
Contributor Author

vlapo commented Jan 24, 2019

Have a logs provided in node-mssql issue:

pool(1): connection #3 created
connection(3): establishing
pool(1): connection #4 created
connection(4): establishing
connection(2): borrowed to request #115
request(115): query SELECT ...
connection(2): released
request(115): completed
connection(2): borrowed to request #116
request(116): query SELECT ...
connection(2): released
request(116): completed
connection(2): borrowed to request #117
request(117): query SELECT ...
connection(2): released
request(117): completed
    ✓ should be able to retrieve the whole tree (313ms)
====> before close
====> after close
connection(3): established
connection(4): established
connection(2): destroying
connection(2): destroyed
connection(3): destroying
connection(3): destroyed

As you can see there is established connection no. 4 and never destroyed. But this behaviour is not occur every time. In our large test suites it is almost every time but in different tests.

I provided failing test case in node-pool and it seem like race condition. Maybe I am totally wrong. Will see.

@dhensby
Copy link
Contributor

dhensby commented Jan 24, 2019

@vlapo interesting - yeah, we work with a medium sized test suite and aren't seeing that problem itself, but I can see that if the pool has created a connection that hasn't established by the time pool.close() is called, it seems to fail to abort or clean that connection up... Which is then lost to the ether :/

@vlapo
Copy link
Contributor Author

vlapo commented Feb 3, 2019

@dhensby We have a some progress here. coopernurse/node-pool#159 (comment) I tested it on my local environment by replacing generic-pool sources in mssql package and it works like a sharm. No hanging DB connections anymore.

@dhensby
Copy link
Contributor

dhensby commented Feb 4, 2019

Let me update the deps on mssql, then :)

@vlapo
Copy link
Contributor Author

vlapo commented Feb 4, 2019

We are all green 💯 🔥 🎉 @dhensby, @sandfox thank you for your effort 🍺

@pleerock
Copy link
Member

pleerock commented Feb 4, 2019

Thank you guys!

@vlapo do we use any api of new mssql that doesn't exist in 4.x? (Is it a breaking change for exist mssql users with 4.x installed?)

@dhensby
Copy link
Contributor

dhensby commented Feb 4, 2019

I can release a 4.x update too, if you like - the change log for 5 is in the mssql repo - there is 1 config change that is different (forced on us by upgrading tedious version) but otherwise tried to keep everything BC

@dhensby
Copy link
Contributor

dhensby commented Feb 4, 2019

4.3.1 tagged too :)

@vlapo
Copy link
Contributor Author

vlapo commented Feb 4, 2019

ok, lets use stable 4.3.1 version instead 5.0.0-beta :-)

@pleerock pleerock merged commit 483c68e into typeorm:master Feb 9, 2019
@vlapo vlapo deleted the travis-enable-mssql branch February 10, 2019 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants