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

Synchronous connection pool initialisation r.connectPool() #60

Open
mishawakerman opened this issue Jun 19, 2020 · 7 comments
Open

Synchronous connection pool initialisation r.connectPool() #60

mishawakerman opened this issue Jun 19, 2020 · 7 comments
Labels
enhancement New feature or request question Further information is requested
Milestone

Comments

@mishawakerman
Copy link

We encountered an issue when migrating our expressjs app from rethinkdbdashrethinkdb-ts. With rethinkdbdash, creating our express app and initialising the database was synchronous:

export const r = require('rethinkdbdash')(config.get('rethinkdb')});

After moving to rethinkdb-ts we needed to run r.connectPool(...) when the app initialised. The (apparent) way to handle this sort of async initialisation is to return an app promise rather than an app and handle it in the thing that attaches the app to a http server. For example:

import { init as initDatabase } from './db';
const app = express();

// ... add all middleware

export const appPromise = Promise.all([
    initDatabase()
]).then(() => {
    return app;
}).catch(err => {
    winston.error("could not initialise application", err)
});

and then in ./bin/www

...
appPromise.then(app => {
  app.set("port", 4000);
  const server = http.createServer(app);
  server.listen(port);
  ...

The problem with this is that it would have required us to change over 100 test files that all import the app for integration tests.

Our "solution" (for which I'm hoping someone can suggest a better approach) was to add an express middleware that runs r.connectPool() the first time it runs and otherwise does nothing:

let isDatabaseInitialised = false;
app.use(async (res, req, next) => {
    if (!isDatabaseInitialised) {
        try {
            await r.connectPool(dbConfig);
            isDatabaseInitialised = true;
        } catch (err) {
            console.error("could not initialise database:", err);
            next(err);
        }
    }
    next();
})

We're not very happy with this method and it adds a (very very) small amount of overhead to every API request but seemed like the easiest way to migrate from rethinkdbdash without having to modify hundreds of files and generally would (if it's possible) love a synchronous version of r.connectPool().

@mishawakerman mishawakerman added enhancement New feature or request question Further information is requested labels Jun 19, 2020
@atassis
Copy link
Collaborator

atassis commented Jul 26, 2020

connectPool should be a Promise because by default it checks if the pool is healthy. I don't think, that this behavior is redundant, so I am closing an issue. And yes, we are going to make more changes to the library, so don't think that this library is strongly based on rethinkdb-dash

@atassis atassis closed this as completed Jul 26, 2020
@atassis
Copy link
Collaborator

atassis commented Jul 26, 2020

Okay, I have reviewed the code and, maybe, we can change an API and create an event when connectPool is ready. I'll discuss this with a team.

@atassis atassis reopened this Jul 26, 2020
@Sven65
Copy link

Sven65 commented May 7, 2021

Has there been any update on this, @atassis?

@atassis
Copy link
Collaborator

atassis commented May 7, 2021

@Sven65 I was working on 3.0 version of library, where r is only used for generating queries, connection logic was moved out of single r object. 1 test was broken, have to fix it, but have no time to finish it. Several improvements were made there, but this is what we have now

@Sven65
Copy link

Sven65 commented May 7, 2021

@Sven65 I was working on 3.0 version of library, where r is only used for generating queries, connection logic was moved out of single r object. 1 test was broken, have to fix it, but have no time to finish it. Several improvements were made there, but this is what we have now

Thanks for the response, I'm loolking forward to v3!

@atassis
Copy link
Collaborator

atassis commented May 11, 2021

Ok, I have prepared the new version in next branch, but will need few more days to write a new documentation from scratch for this and deploy as alpha version, our CI is not ready for alpha's yet

@atassis atassis added this to the Version 3.0 milestone May 11, 2021
@atassis
Copy link
Collaborator

atassis commented Jun 14, 2021

at this very moment you are able to synchronously instantiate the connection pool by using rethinkdb-ts@next

import { MasterConnectionPool } from "rethinkdb-ts/lib/connection/master-pool";

I am working on making API clear, and also maybe adding some more stuff, thats why still not latest tag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants