Solution to / suggestion for "cannot read property 'close' of undefined"

The chapter 1 lecture on MongoClient has a problem which seems to be tripping quite a few people up, me included.

The second and fourth tests in mongoclient.spec.js supply a connectionTimeoutMS value of 200 in the options passed to the MongoClient.connect method. So if a connection to the cluster hasn’t been established within 200 milliseconds, MongoClient.connect throws an error. Fair enough. However, the way in which the tests are structured:

let testClient
try {
  testClient = await MongoClient.connect( ... )
  // ... 
} catch (e) {
  expect(e).toBeNull()
} finally {
  testClient.close()
}

… isn’t particularly helpful in this scenario. MongoClient.connect throws an error, so control is passed to the catch block (I verified this by adding a console.log to that block), and then to the finally block to ensure that the connection is always closed once we’ve finished with it, even if we’ve finished with it in a messy way by falling flat on our metaphorical face part way through our data access code. This also strikes me as a sensible thing to do, we probably don’t want connections hanging around if they’re never going to be used again because they’re going to be consuming resources somewhere which could be put to better use.

Except the connection we’re trying to close doesn’t exist yet, because the MongoClient.connect call which was going to create it threw an error, so we now have another error:

TypeError: Cannot read property 'close' of undefined

And it gets worse. Once Jest sees that latest error, it doesn’t appear to report the original error thrown by MongoClient.connect which indicates that the problem is a timeout connecting to the cluster, leaving a rather baffled student wondering whether to add a bunch of console.logs to the test to figure out what’s really happening (when we’re told not to change the unit tests).

Anyway, the fix which worked for me was to change the value of connectTimeoutMS from 200 to 1000 so that I didn’t get a timeout. The value which works will be different for different people, depending on where the cluster is hosted, how powerful the hardware is, how much work it’s doing for other people at the same time, and how fast the network connection between the client and the cluster is (and probably a million other factors that I haven’t thought of). I’d say just keep increasing it until either it works or you get a different error.

I’ve already used the “report an issue” facility on that lecture page to suggest increasing this value from 200 to 1000 (or whatever), but I also have a suggestion to improve the error handling to reduce the confusion for students who face this problem, which I don’t think I can express in 500 characters.

Rather than a single try/catch/finally to handle all errors which can happen, have one try/catch to handle errors in creating the connection, and then within that try block, have another try/catch/finally to handle errors which happen once the connection has been opened and to ensure that the connection is always closed once we’re finished with it.

Here’s a function to illustrate what I mean:

/**
 * Demonstration of exception handling around MongoDB data access code,
 * showing the use of one try/catch to handle failure to connect to Mongo and
 * another try/catch/finally to ensure that the connection is closed if an
 * exception is thrown whilst the connection is open.
 * @param  {string} uri URI of the Mongo cluster to connect to.
 * @param  {integer} timeoutInMS Number of milliseconds to wait for a connection
 * to Mongo to be established before timing out.
 * @param  {boolean} throwArbitraryException True to throw an exception whilst
 * the connection to Mongo is open.
 * @return {integer} Number of documents in the mflix database's movies collection.
 */
const countMovies = async (uri, timeoutInMS, throwArbitraryException) => {
    try {
        const options = {
            connectTimeoutMS: timeoutInMS,
            useNewUrlParser: true
        };

        const client = await MongoClient.connect(uri, options);

        // if we've got this far then we've opened a connection to the database
        try {
            const mflixDB = client.db("mflix");
            const movies = mflixDB.collection("movies");
            const numMovies = await movies.countDocuments({});

            if (throwArbitraryException) {
                throw new Error("Arbitrary exception");
            }

            return numMovies;
        } catch (e) {
            throw new Error(`Exception whilst querying the database: ${e}`);
        } finally {
            // we've opened a connection so we need to close it now
            client.close();
        }
    } catch (e) {
        // We don't have a connection to close, so don't try to close it, just
        // report that there was a problem trying to connect.
        throw new Error(`Exception in MongoClient.connect(): ${e}`);
    }
}

And here are my unit tests, exercising the 3 paths through the function.

describe("Simons MongoClient.connect() exception test", async () => {
    const clusterUri = process.env.MFLIX_DB_URI;
    const impatientTimeout = 1;
    const patientTimeout = 1000;

  it("Happy path", async () => {
      const numMovies = await countMovies(clusterUri, patientTimeout, false);
      expect(numMovies).toBe(45993);
  })

  it("Tells us when the connectionTimeoutMS parameter is too small", async () => {
      try {
          await countMovies(clusterUri, impatientTimeout, false);
      } catch (e) {
          expect(e).not.toBeNull();
          expect(e).not.toBeUndefined();
          expect(e.message).toContain("timed out");
      }
  })

  it("Tells us when some other exception has been thrown after the connection is open", async () => {
      try {
          await countMovies(clusterUri, patientTimeout, true);
      } catch (e) {
          expect(e).not.toBeNull();
          expect(e).not.toBeUndefined();
          expect(e.message).toContain("Arbitrary exception");
      }
  })
})

Now I’m not an expert in JavaScript, I’ve just learned quite a lot about it in the past year, so there may be many reasons why doing things this way isn’t a good idea. I’d like to hear people’s thoughts either way.

Apologies for the long post, I hope it proves to be useful to someone :slight_smile:

2 Likes

Hi Simon, your solution worked for me. I had the same problem in tests 2 and 4.
It was solved increasing the time.

Thank you very much!

It worked for me too, and it absolutely clarified me the problem.

Thanks for helping.

1 Like