javascript callback is called twice in an impossible invocation

Question

I built a TS, MongoDB Client wrapper. for some reason when I call the function that gets the connection, its callback is called twice.

There are 2 calls in total to the get() function, 1 before the export as you can see and another from a mocha test.

I am pretty new to TS and JS in general, but this seems a bit off.

    import {Db, MongoClient} from "mongodb";
    import {MongoConfig} from '../config/config'
    class DbClient {
        private cachedDb : Db = null;
        private async connectToDatabase() {
            console.log('=> connect to database');
            let connectionString : string = "mongodb://" + MongoConfig.host + ":" + MongoConfig.port;
            return MongoClient.connect(connectionString)
                .then(db => {
                    console.log('=> connected to database');
                    this.cachedDb = db.db(MongoConfig.database);
                    return this.cachedDb;
                });
        }
        public async get() {
            if (this.cachedDb) {
                console.log('=> using cached database instance');
                return Promise.resolve(this.cachedDb);
            }else{
                return this.connectToDatabase();
            }
        }
    }
    let client = new DbClient();
    client.get();
    export = client;

where the console output is:

=> connect to database
=> connected to database
=> connected to database

Any particular reason this is misbehaving?

Side note: there's a race condition where `get` could be called multiple times before `this.cachedDb` is set which would lead to multiple connections/instance of `Db` being created. This can be avoided by assigning the promise of `connectToDatabase` to a private property then returning that promise in `get` (it should probably handle failures some way though). — David Sherret, Jun 10 '19 at 14:04
Your Promise logic is flawed. in addition to what David suggests, the `get()` method returns a Promise wrapped in a promise or a resolved promise (`this.cachedDb`) wrapped in a promise. But then don't use either. You return from the promise from `MongoClient.connect()` but never use it (other then returning from `get`). Finally, you use then on the promise and return a a value that never get's used. Perhaps you should review the very simple design in the [Mongo Guides](http://mongodb.github.io/node-mongodb-native/3.2/reference/ecmascriptnext/connecting/) — Randy Casburn, Jun 10 '19 at 14:10
@RandyCasburn I don't see any promises wrapped in a promise (promises that return a promise) in that code. Maybe I'm missing something? — David Sherret, Jun 10 '19 at 14:23
@David - `connectToDatabase()` is async and returns a Promise object, that Promise object is returned directly by `get()`, also an async function which returns a Promise wrapped in a Promise. — Randy Casburn, Jun 10 '19 at 14:27
@Gleeb - check out this [gist](https://gist.github.com/randycasburn/6fe40c1b935ee1ac5beba32dbd819371) It is your class written in a much cleaner way. — Randy Casburn, Jun 10 '19 at 14:27
@RandyCasburn ah yeah, I missed the `async` keywords on the methods. Thanks! — David Sherret, Jun 10 '19 at 14:29
Thank you all for replying with a code review on these promises, I am used to Scala and this shit just can't happen there. — Gleeb, Jun 11 '19 at 13:15

David Sherret · Accepted Answer · 2019-06-10T17:39:12.403

There are 2 calls in total to the get() function, 1 before the export as you can see and another from a mocha test.

I suspect the output has an additional => connect to database. As I said in the comments: There's a "race condition" where get() could be called multiple times before this.cachedDb is set which would lead to multiple connections/instances of Db being created.

For example:

const a = client.get();
const b = client.get();

// then
a.then(resultA => {
    b.then(resultB => {
        console.log(resultA !== resultB); // true
    });
});

Solution

The problem can be fixed by storing the promise as the cached value (also, no need to have the async keyword on the methods as Randy pointed out, as there's no values being awaited in any of the methods so you can just return the promises):

import {Db, MongoClient} from "mongodb";
import {MongoConfig} from '../config/config'

class DbClient {
    private cachedGet: Promise<Db> | undefined;

    private connectToDatabase() {
        console.log('=> connect to database');
        const connectionString = `mongodb://${MongoConfig.host}:${MongoConfig.port}`;
        return MongoClient.connect(connectionString);
    }

    get() {
        if (!this.cachedGet) {
            this.cachedGet = this.connectToDatabase();

            // clear the cached promise on failure so that if a caller
            // calls this again, it will try to reconnect
            this.cachedGet.catch(() => {
                this.cachedGet = undefined;
            });
        }

        return this.cachedGet;
    }
}

let client = new DbClient();
client.get();
export = client;

Note: I'm not sure about the best way of using MongoDB (I've never used it), but I suspect connections should not be so long lived as to be cached like this (or should probably only be cached for a short time and then disconnected). You'll need to investigate that though.

I used your code snippet thank you it's actually very concise and correct. About the long lived session, I used this guide as a basis for my code (as I am building for Lambda). https://docs.atlas.mongodb.com/best-practices-connecting-to-aws-lambda/ — Gleeb, Jun 11 '19 at 13:15
@Gleeb ok, cool. As long as it's what they recommend. I wonder if you need to deal with reconnecting on unexpected disconnections, but perhaps that's all handled internally. I feel like they would have mentioned that if you needed to. Also, I see where you got your original code from! — David Sherret, Jun 11 '19 at 14:01

javascript callback is called twice in an impossible invocation

1 Answers1