Automattic / mongoose

MongoDB object modeling designed to work in an asynchronous environment.
https://mongoosejs.com
MIT License
26.92k stars 3.84k forks source link

Properly dispose of closed / failed connections created with mongoose.createConnection() #11827

Closed deistermatheus closed 2 years ago

deistermatheus commented 2 years ago

Do you want to request a feature or report a bug? Possible bug, but should be posed more as a question

What is the current behavior?

We've checked long running docker containers that use this specific file (see steps to reproduce) and found out that they keep hundreds of ESTABLISHED connections to the replica set, even though our maxPoolSize is 1. This specific service becomes idle at many times during the day and might (should?) trigger socket timeouts from time to time. Our database connection file is supposed to catch "disconnect" and "error" events and create a new connection, but I suspect these listeners are not set up correctly or do not work as intended. Should socket timeouts on the underlying connection emit events in the library code, is there a way to listen (i.e using MongoClient directly)?

By simulating a network failure by turning wifi off and on for a time lower than server selection timeout, I'm seeing an increase in connections when our code performs reconnection, which indicates that we are not clearing up resources correctly. I checked the mongoose object and the array of connections is growing with each reconnect, which indicates that we need to perform some kind of cleanup.

If the current behavior is a bug, please provide the steps to reproduce.

const mongoose = require('mongoose')

let connection = null

process.on('exit', () => {
    mongoose.disconnect()
})

class MongoDB {
    static getUri() {
        if (process.env.DB_MONGO_URI) {
            return process.env.DB_MONGO_URI
        }

        let mongoUri = 'mongodb://'

        if (process.env.DB_MONGO_USER && process.env.DB_MONGO_PASSWORD) {
            mongoUri += `${process.env.DB_MONGO_USER}:${encodeURIComponent(process.env.DB_MONGO_PASSWORD)}@`
        }

        mongoUri += process.env.DB_MONGO_HOST

        if (process.env.DB_REPLICASET) {
            mongoUri += process.env.DB_REPLICASET
        }

        return mongoUri
    }

    static createConnection() {
       const options = {
            serverSelectionTimeoutMS: 15000,
            socketTimeoutMS: 1000, // does this drop the idle connection?
            connectTimeoutMS: 15000,
            maxPoolSize: 1,
            minPoolSize: 1,
            family: 4,
            maxIdleTimeMS: 1000, // does this drop the idle connection?
            keepAlive: false
        }

        return mongoose.createConnection(MongoDB.getUri(), options)
    }

    static startEvents() {
        connection.on('error', error => {
            console.error(`Error in MongoDb connection: ${error}`)
            mongoose.disconnect()
        })

        connection.on('disconnected', () => {
            console.log('MongoDB disconnected!')
            connection = MongoDB.createConnection()
        })

        connection.on('connected', () => console.log('MongoDB connected!'))
        connection.on('reconnected', () => console.log('MongoDB reconnected!'))
        connection.on('reconnectFailed', () => console.log('MongoDB reconnect failed!'))
        connection.on('close', () => console.log('MongoDB close!'))
    }

    static getOrCreateConnection() {
        if (!connection) {
            connection = MongoDB.createConnection()
            MongoDB.startEvents(connection)
        }

        return connection
    }
}

module.exports = MongoDB
const MongoDB = require('../databases/mongodb')

const connection = MongoDB.getOrCreateConnection()

const someModel = connection.model('name', Schema, 'pluralName')

module.exports = someModel

What is the expected behavior?

With the right combination of options, the OS closes inactive sockets and nodejs/mongoose/mongo client reopens the connection, without leaking inactive connections. There's no problem with leaving one connection open for the lifetime of the microservice, however, having hundreds of outgoing ghost connections kills our tier limits in MongoDb Atlas and I suspect something is wrong with the way our code talks to mongo.

Might be an issue with our own code too, in that case, could someone point out the documentation / correct setup to have this kind of logic implemented? This pull request might be related.

What are the versions of Node.js, Mongoose and MongoDB you are using? Note that "latest" is not a version. Mongoose: 6.2.3 Node.js; 16.13 MongoDB: 4.4.9-19 Community

deistermatheus commented 2 years ago

I think we've found the issue. Any failures get handled as follows:

 connection.on('error', error => {
            console.error(`Error in MongoDb connection: ${error}`)
            mongoose.disconnect()
        })

        connection.on('disconnected', () => {
            console.log('MongoDB disconnected!')
            connection = MongoDB.createConnection()
        })

However, mongoose reconnects replica sets automatically. What the code is doing is essentially pushing a new connection to the mongoose.connections internal array and binding new listeners. Whenever a network failure or connection error occurs, a new connection is pushed and previous connections get reconnected. This explains dangling connections in long running microservices.

I don't think it is a mongoose issue, the root cause looks like a developer mistake.

vkarpov15 commented 2 years ago

Why are you creating a new connection on the 'disconnected' event? You don't have to do that, nor should you. When you try to execute any database operation (find, save, etc.) Mongoose and the underlying MongoDB driver will try to connect for up to serverSelectionTimeoutMS milliseconds, regardless of the current connection state, unless you explicitly disconnect() or close(). The 'disconnected' event is for debugging and monitoring, you don't need to create a new connection yourself.