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

Add connectionLifetime to pool options #2195

Closed
wants to merge 1 commit into from

Conversation

dicearr
Copy link

@dicearr dicearr commented Mar 14, 2019

This option is specially useful when connecting to serverless databases, in which the instances are turned off when there is no connection alive.

Fixes #1276

@dicearr
Copy link
Author

dicearr commented Mar 14, 2019

Two tests are failing in one of the Travis jobs.

Node version: 6.16
DOCKER_MYSQL_TYPE=mariadb
DOCKER_MYSQL_VERSION=10.2

Files are:

test/unit/pool-cluster/test-connection-error-remove.js
test/unit/pool-cluster/test-connection-rr-error.js

I think those failures are unrelated to the changes but I am not hundred percent sure. The AppVeyor build for node v6.16 is passing.

@dougwilson
Copy link
Member

Thank you. I will check out the CI failures 👍

@dolsup
Copy link

dolsup commented May 8, 2019

I think the timeout should be set every time after pool connection is released, not at the creation. and it have to be clear when pool connection is acquired again after release.

@dicearr
Copy link
Author

dicearr commented May 8, 2019

That would be something like idleConnectionTimeout rather than connectionLifetime, right?

I have had time to check the PR again and you're completely right. In the way it is currently built if the connection expires while is being used the socket gets destroyed causing an error to be thrown. I think that unless I could gracefully close the connection, your suggestion is the way to go. Was that the reasoning behind your comment?

I have found #1749 which seems to be an attempt for implementing the idleConnectionTimeout, and #962 which is the related issue. The PR is stalled for no apparent reason.

@dougwilson Could you please share your thoughts about this? If the idleConnectionTimeout is the way to proceed I will close this PR and open a new one.

@dolsup
Copy link

dolsup commented May 8, 2019

@dicearr You said exactly what I meant.
In my case I made a monkey patch which is something like idleConnectionTimeout for my production which is built using microservice architecture and needs flexible pool size for each service. because the max connection capacity of my database is so small. I hope it goes to official feature.

And you can consult this PR.
sidorares/node-mysql2#736

@dicearr
Copy link
Author

dicearr commented May 10, 2019

Closed in favor of #2218.

@dicearr dicearr closed this May 10, 2019
@dicearr dicearr deleted the connection-lifetime branch May 10, 2019 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Connections in pool have an infinite lifetime
3 participants