needle-innovision / nestjs-tenancy

Multi-tenancy approach for nestjs - currently supported only for mongodb with mongoose
MIT License
187 stars 58 forks source link

New DB connection on every request? #44

Open raphaelarias opened 1 year ago

raphaelarias commented 1 year ago

Hi @sandeepsuvit.

I was doing some stress tests as we prepare to go live, and maybe I did something wrong, but it seems like for every new request, it's creating a new database connection

image
    TenancyModule.forRoot({
      tenantIdentifier: 'X-TENANT-ID',
      options: () => { },
      uri: (tenantId: string) => {
        console.log('connecting!?', tenantId);
        return process.env.TENANT_MONGO_DB_CONNECTION_STRING.replace(
          '{{TENANT_ID}}',
          tenantId,
        );
      },
    }),

image

I ran a script that calls a URL, the URL return a 500 code, which is okay. But wheneve r I ran the script, the connection to my MongoDB Atlas instance skyrockets.

Would you know why? I saw that it's supposed to cache it: https://github.com/needle-innovision/nestjs-tenancy/issues/12. But now I'm puzzled, and trying to find the root cause of the problem.

raphaelarias commented 1 year ago

Okay, maybe I was too fast on opening an issue. And I apologesie, it doesn't seem that the line "connecting!? tenantId" is not being called after a while, I'm assuming it's using the cached connection.

But one question. Have you experienced yourself or seen it in production, a problem with too many connections open? In this case I think I had 37 tenants as a test, I'm afraid it's going to explode in number of connections very quickly.

I wonder as the OP in issue #12 if useDb() would avoid that?

raphaelarias commented 1 year ago

Considering that in production, each tenant would generate at least 6 connections (maxPoolSize + 1) * number_of_nodes, I wonder if anyone had problems before.

Source: https://www.mongodb.com/community/forums/t/poolsize-connection-count/113029

raphaelarias commented 1 year ago

Or close connections after inactivity time to clean up the pool. It seems to me though, that useDb doesn't need as many connections.

Source: https://stackoverflow.com/questions/57345147/multi-tenant-mongodb-mongo-native-driver-connection-pooling

elmapaul commented 1 year ago

@raphaelarias I really respect your concern, cz wondering as well how it will behave in prod mode with N users/connections. So pity, this library is not actively supported. No complains, just a thought.

Quick tip: you can easily wrap up your all messages in one with edit :)

sandeepsuvit commented 1 year ago

@raphaelarias @elmapaul apologies for replying late on this since I only work on this during my spare time. Also @raphaelarias regarding your query the connections are stored on a map and reused if found. Refer to this line in the code.

// Check if tenantId exist in the connection map
    const exists = connMap.has(tenantId);

    // Return the connection if exist
    if (exists) {
      const connection = connMap.get(tenantId) as Connection;

      if (moduleOptions.forceCreateCollections) {
        // For transactional support the Models/Collections has exist in the
        // tenant database, otherwise it will throw error
        await Promise.all(
          Object.entries(connection.models).map(([k, m]) =>
            m.createCollection(),
          ),
        );
      }

      return connection;
    }

    // Otherwise create a new connection
    const uri = await Promise.resolve(moduleOptions.uri(tenantId));
    // Connection options
    const connectionOptions: ConnectionOptions = {
      useNewUrlParser: true,
      useUnifiedTopology: true,
      ...moduleOptions.options(),
    };

    // Create the connection
    const connection = createConnection(uri, connectionOptions);

    // Attach connection to the models passed in the map
    modelDefMap.forEach(async (definition: any) => {
      const { name, schema, collection } = definition;

      const modelCreated: Model<unknown> = connection.model(
        name,
        schema,
        collection,
      );

      if (moduleOptions.forceCreateCollections) {
        // For transactional support the Models/Collections has exist in the
        // tenant database, otherwise it will throw error
        await modelCreated.createCollection();
      }
    });

    // Add the new connection to the map
    connMap.set(tenantId, connection);

    return connection;
raphaelarias commented 1 year ago

I'm sorry for the SPAM :p

Indeed, I can see here that the connection is opened only on first access, and then re-used. Which is great.

@sandeepsuvit What are your thoughts about .useDB() to lower the number of connections?

I appreciate your support and expertise, this package has been very useful, so if there's any way I can help and support, let me know.

sandeepsuvit commented 1 year ago

@raphaelarias sure, i am happy for any support. To be frank i haven't really tried the .useDb() to lower the number of connections yet. Would really appreciate it if you can give it a try on your free time and raise a PR.

raphaelarias commented 1 year ago

Yep! I'll give it a shoot, share here what I learn, and hopefully create a PR.