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

New DB connection on every request? #44

Open
raphaelarias opened this issue Dec 22, 2022 · 8 comments
Open

New DB connection on every request? #44

raphaelarias opened this issue Dec 22, 2022 · 8 comments

Comments

@raphaelarias
Copy link

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: #12. But now I'm puzzled, and trying to find the root cause of the problem.

@raphaelarias
Copy link
Author

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
Copy link
Author

raphaelarias commented Dec 22, 2022

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
Copy link
Author

raphaelarias commented Dec 22, 2022

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
Copy link

@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
Copy link
Contributor

@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
Copy link
Author

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
Copy link
Contributor

@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
Copy link
Author

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

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

No branches or pull requests

3 participants