Closed UziTech closed 4 years ago
I like this. It will be even better when support to Node 6 is dropped and async
/await
will be usable
There are a few other places Promises could be used but this is the only one I know is failing
This is intentional design choice. When a resource is destroyed from pool we remove it from available/in-use objects and issue destroy on it.
There is no need to wait for actual network connection termination, is there?
Then there is no way to wait for the actual disconnect and the beforeDisconnect
hook actually happens after await sequelize.close()
I would expect await sequelize.close()
to actually wait until the connections are closed.
beforeDisconnect
still executes before resource destruction and afterDisconnect
is called after that in correct sequence. There is no need for them to depend upon sequelize.close
The problem is there is no way to await for everything to be done.
Can you explain your usecase? After sequelize.close
pool will shutdown and issue destroy for all connections. Once connections are disconnected REPL will close the process safely
The place I noticed it was when testing our database. There is no way to tell when the test is complete. If there is no connection afterDisconnect
is never called and if there is a connection await sequelize.close()
won't be the end of the test since beforeDisconnect
and. afterDisconnect
still haven't run.
Also if disconnect fails it won't be caught by await sequelize.close()
or anything for that matter. It will just be an unhandled promise rejection, which will terminate the node process in the future.
@sushantdhiman any feedback?
@sushantdhiman is this moving forward?
@sushantdhiman anything you want me to change to get this moving? It seems to cause issues with flaky tests and there is no current way around it without monkey patching.
Hi @UziTech, can you please help me understand something? How can we be sure that this PR will not have drastic repercussions? I agree with you that the correct behavior seems to be the one you are suggesting, but since this package has been used for quite a while now, how can we know that merging this PR will not have drastic consequences for (some of) sequelize users?
The only thing this does is waits on destroying the connection before resolving the await sequelize.close()
Promise.
If someone doesn't want to wait for it to be destroyed they can just not await the sequelize.close()
and it would do the same thing it does without this PR.
If someone writes await sequelize.close()
I'm guessing they want to wait until sequelize closes the connections. Which is not what currently happens.
Is this going to get merged so we can stop having flaky tests?
I rebased and added a test
:tada: merged within a year of opening the PR 🤣
Thanks for your patience, before adopting to typescript I wasn't sure if everything will work as this is forked code mainly. But now I can accept this change.
I'll release it soon as 6.0.0
in Sequelize the destroy function returns a promise that is not awaited by sequelize-pool.
When calling
await sequelize.close()
it will now wait for the destroy function's promise to resolve.without this PR
with this PR