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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mongoose reconnect #4315

Closed
wants to merge 7 commits into from
Closed

Mongoose reconnect #4315

wants to merge 7 commits into from

Conversation

AHgPuK
Copy link
Contributor

@AHgPuK AHgPuK commented Oct 22, 2019

This patch will restore a lost connection to MongoDB.
Checked on both standalone and replica set environments.
Please, put an attention that mongoose option reconnectTries doesn't have an effect on replica set.
The only way to restore a connection in this case is to use Mongoose.connect call

Related to MongoError: server instance pool was destroyed #4188

Description of what you did:

My PR is a:

  • 馃挜 Breaking change
  • 馃悰 Bug fix
  • 馃拝 Enhancement
  • 馃殌 New feature

Main update on the:

  • Admin
  • Documentation
  • Framework
  • Plugin

Manual testing done on the following databases:

  • Not applicable
  • MongoDB
  • MySQL
  • Postgres
  • SQLite

This patch will restore a lost connection to MongoDB.
Checked on both standalone and replica set environments.
Please, put an attention that mongoose option reconnectTries doesn't have an effect on replica set.
The only way to restore a connection in this case is to use Mongoose.connect call
@alexandrebodin
Copy link
Member

@AHgPuK Thanks for this PR.

Can you references some discussions (in the mongoose repo or something like that) as to why the auto reconnect isn't handle directly with mongoose ?

@AHgPuK
Copy link
Contributor Author

AHgPuK commented Oct 22, 2019

@AHgPuK Thanks for this PR.

Can you references some discussions (in the mongoose repo or something like that) as to why the auto reconnect isn't handle directly with mongoose ?

First of all, it fixes MongoError: server instance pool was destroyed #4188

In case of replica set (I use a lot for my projects) mongoose tries to reconnect only few times. Perhaps during 1 minute or less.
After this time mongoose will not reconnect itself anymore.
reconnectTries option did not make any sense.
I looked at Mongoose docs

Make reconnectTries and reconnectInterval readable from configs
@@ -87,17 +89,33 @@ module.exports = function(strapi) {
connectOptions.dbName = database;
connectOptions.useCreateIndex = true;
connectOptions.useUnifiedTopology = useUnifiedTopology || 'false';
connectOptions.reconnectInterval = reconnectInterval || defaults.reconnectInterval;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the defaults are already set on line 65 (coming from strapi.config.hook.settings.mongoose)

try {
await connect();

instance.connection.on('disconnected', function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please use arrow functions in this code. Don't forget to implement the reconnectTries logic too :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should leave reconnectTries logic under the mongoose hood.
As I tried to explain I cant find a reason to stay disconnected.
If you have a scenario where a strapi project could be irresponsible forever I would change my mind.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SImple example: a docker container. if you disconnect it should exit instead of keeping alive for eternity hence the retry counter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will not exit on disconnect. Too many listeners are bound including http port binding etc.
I use docker containers in production. No similar scenario.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are missing my point, whether you want a docker container to be killed or not is irrelevant.

What I am saying is that a user should be able to control that :) e.g retry 5 times then do sth like:

connection.on('someFinalEvent', ()= > { /* do sth */ }) 

or anything else the user wants to do.

1. Remove not needed default values assignments
2. Use arrow function
@AHgPuK AHgPuK closed this Oct 23, 2019
@alexandrebodin
Copy link
Member

@AHgPuK Are you going to open a new PR or sth ?

@AHgPuK
Copy link
Contributor Author

AHgPuK commented Oct 23, 2019

@AHgPuK Are you going to open a new PR or sth ?

No. Never mind.
I can't waste my time...

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.

None yet

3 participants