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

Updated the disconnect() method to wait until the connection has been… #9911

Conversation

@AlastairTaft
Copy link
Contributor

@AlastairTaft AlastairTaft commented Sep 12, 2018

… ended.

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)? No, but the same two tests aren't passing that are on master.
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

We're using sequelize on AWS lambda with the postgres dialect and the script execution is halted before the connection is closed which causes weird issues with database connections. This change ensures that the connection is properly closed before the execution ends.

Helps with #8468.

connection.end();
resolve();
connection.end()
.then(() => resolve());

This comment has been minimized.

@sushantdhiman

sushantdhiman Sep 13, 2018
Contributor

return Promise.fromCallback(callback => connection.end(callback));
@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Sep 13, 2018

I don't think this will solve issue you are facing. I really want pool to shutdown when event loop is empty but node-pool implementation is such it keep process alive thus preventing clean shutdown.

@AlastairTaft
Copy link
Contributor Author

@AlastairTaft AlastairTaft commented Sep 13, 2018

Do you know if fixing that is on node-pool/generic-pool's roadmap?
EDIT: Based on your comment on the other thread I think you're thinking of either switching to generic-pool v2 or rolling a different pooling library.

@sushantdhiman sushantdhiman merged commit 56d5fee into sequelize:master Sep 13, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants