CODEWITHSUNDEEP
node.jsmongodb
Arman
Arman

Reputation: 2925

Mongodb Node Driver - does it really matter when I call db.close() or am I getting lucky?

I've found that db.close() does not seem to close until all reads or writes have finished regardless of when it is called. This is actually what I would want, but I want to make sure this is the intended behavior and not just me getting lucky. I have some code like this:

...
{
  collection.insertMany(...).then(()=> { console.log("inserted") })
  collection.deleteMany(...).then(()=> { console.log("deleted") })
}).then(() =>
{
  console.log("will close");
  client.close();
}).catch((reason) => { console.log(reason) });

I get output like:

will close
deleted
inserted

but no errors and upon further testing, the proper records were in fact inserted and deleted. In the documentation we see that they call db.close() in whatever the final callback is, but I did not do it then. Is this reliable? I'm not sure how to restructure my code to make it more clear that the close is issued after both the insertMany and deleteMany - perhaps I should be using promise.all.

EDIT So both of the answers below make sense to me as to how they would guarantee the connection closing at the appropriate time, with Neil Lunn's suggestion to use bulkWrite seeming ideal for performance reasons. I'll likely go the bulkWrite route in my application for the time being for the sake of good practice, but I still have some confusion regarding my original question...

To me it seems the code above issues insertMany and deleteMany before client.close() regardless, so I would think mongodb is aware that there are pending operations, so will close appropriately. Can someone give me an example scenario as to how client.close() can be called prematurely? If the assumption is that an error in either of the operations can result in client.close() being called early, then I propose adding 

{
  collection.insertMany(...).then(()=> { console.log("inserted") })
  collection.deleteMany(...).then(()=> { console.log("deleted") })
}).then(() =>
{
  console.log("will close");
}).catch((reason) => { console.log(reason) })
  .then(() => { client.close() })

Upvotes: 0

Views: 163

Answers (2)

Neil Lunn
Neil Lunn

Reputation: 151112

In all honesty, you're not only "getting lucky" with the actions happening to resolve before you call client.close() but also there could be a "potential" problem with the two statements running in parallel.

Ideally you would instead use bulkWrite() and make a "single" request with both actions contained in it. The "single request" also has a "single response", therefore no issue with awaiting resolution of multiple promises:

collection.bulkWrite(
  [
    ...listOfInsertDocuments.map(document => ({ "insertOne": { document } })),
    { "deleteMany": { "filter": filterCondition } }
  ]
)
.then(() => console.log("will close"))
.catch(e => console.error(e))
.then(() => client.close());

The default action of bulkWrite() is that operations are "ordered" and execute serially in the same order in which they were presented in the array of "actions" presented. The Array.map() used here to create the insertOne actions is in fact exactly what the insertMany() method actually does in the underlying driver implementation.

In fact ALL such driver methods actually call the underlying "Bulk API" methods as the modern way of doing things. The bulkWrite() explicitly abstracts from the actual "Bulk API" in order to gracefully downgrade the requests to the "legacy" API when connecting to a MongoDB instance earlier than MongoDB 2.6.

The other way to do this is "parallel" execution by explicitly setting "ordered": false:

collection.bulkWrite(
  [
  ...listOfInsertDocuments.map(document => ({ "insertOne": { document } })),
  { "deleteMany": { "filter": filterCondition } }
  ],
  { "ordered": false }
)
.then(() => console.log("will close"))
.catch(e => console.error(e))
.then(() => client.close());

That deliberately means that none of the actions are dependent on the others completing "first". So if indeed you are "inserting" something which you are also "deleting", then there is no guarantee which will happen in any particular order.

Aside from "parallelism", the general purpose of "ordered": false is to allow continuation "on error". In such a case, all "batched" actions are actually attempted and in the exception response you get detail telling you at which "index positions" in the array of provided actions there was any failure.

The behavior is then "sort of" done in "mimicry" by the following Promise.all() invocation:

Promise.all([
  collection.insertMany(..., { "ordered": false }).then(()=> { console.log("inserted") }),
  collection.deleteMany(...).then(()=> { console.log("deleted") })
]).then(() => console.log("will close"))
.catch(e => console.error(e))
.then(() => client.close());

That of course executes in "parallel" but fires "multiple requests" which creates a greater overhead with back and forth communication to the server. The purpose of the earlier examples is to avoid that and therefore a better option.

And for "completeness" you can always of course "chain" the promises:

  collection.insertMany(...)
    .then(() => console.log("inserted")),
    .then(() => collection.deleteMany(...)
    .then(()=> console.log("deleted"))
    .then(() => console.log("will close"))
    .catch(e => console.error(e))
    .then(() => client.close());

In that way "everything" is serial in execution, but of course they all fire separate requests.

So whilst there are ways to handle "awaiting" the Promise resolution, it's usually better to make "one call" and just await that "Bulk" response instead.

Upvotes: 1

Sven
Sven

Reputation: 5265

You need to wait for the queries to complete. You can do so by returning Promise.all([query1, query2]), which will resolve when both promises returned by the queries are fulfilled. Additionally, use the Promise's .finally()-method to ensure client.close() is called, even if there is an error. With your current code, if an error occurs the client.close() will not be called.

...
{
  return Promise.all([
    collection.insertMany(...).then(()=> { console.log("inserted") }),
    collection.deleteMany(...).then(()=> { console.log("deleted") })
  ]);
}).then(() => {
  console.log("will close");
}).catch((reason) => {
  console.log(reason);
}).finally(() => {
  client.close();
});

Upvotes: 1

Related Questions