chore(cli-repl): return a cached promise from .close() MONGOSH-1943

[addaleax]

This is mostly just making code a bit more resilient against cases in which we would unintentionally call .close() twice.

[nirinchev]

We can drop the async now, can't we?

[addaleax]

I guess so – would we prefer that? I still like that it's an easy visual indication that this is an async function

[nirinchev]

I guess it's quite inconsequential, so definitely not a hill I'm willing to die on. I'm not as familiar with the internals of async-await in JS, but if it's anything similar to C#, adding an async modifier results in unnecessary allocations and it means that we always return a new promise wrapping closingPromise (i.e. close() === close() will always return false). Granted, this is not on a hot path and I don't see a practical reason to compare the return values for strict equality here, so doesn't really matter - it's more of a general preference for consistency to avoid the async modifier when returning a promise.

Your call - I'm fine either way, just seeing the code triggered my C#-induced OCD 😅

[gagik]

Interesting, it is similar. Coming from JavaScript world before I always saw async as syntax sugar for nicer way to write Promises but apparently it does make a difference, although indeed that subtle equality which won't really matter here. If you are curious, from MDN:

Note that even though the return value of an async function behaves as if it's wrapped in a Promise.resolve, they are not equivalent. An async function will return a different reference, whereas Promise.resolve returns the same reference if the given value is a promise. It can be a problem when you want to check the equality of a promise and a return value of an async function.

const p = new Promise((res, rej) => {
  res(1);
});

async function asyncReturn() {
  return p;
}

function basicReturn() {
  return Promise.resolve(p);
}

function basicReturn2() {
  return p;
}

console.log(p === basicReturn()); // true
console.log(p === basicReturn2()); // true
console.log(p === asyncReturn()); // false

[addaleax]

Yeah, it does allocate a new promise each time the function is being called. And in the early days of async/await, this used to be a point of contention because it did come with nontrivial performance impact (although I think that was more about the fact that the extra microtask queue ) – but that should be something that all major JS engines have addressed by now.

If we go for consistency, I'd honestly aim for using async even if it's redundant – it's just a bit easier to immediately see what's async and what not that way. And if you want a piece of mongosh-specific context, there is https://jira.mongodb.org/browse/MONGOSH-655, which was prompted by the fact that function foo(): Promise<void> { return bar(); } is guaranteed to return a Promise only if bar is also implemented in TS and actually implements the right signature, which wasn't the case for the java-shell implementation back then 🙂