Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Dec 13, 2024
+46 −48
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 46 additions & 48 deletions packages/cli-repl/src/cli-repl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ export class CliRepl implements MongoshIOProvider {
toggleableAnalytics: ToggleableAnalytics = new ToggleableAnalytics();
warnedAboutInaccessibleFiles = false;
onExit: (code?: number) => Promise<never>;
closing = false;
closingPromise?: Promise<void>;
isContainerizedEnvironment = false;
hasOnDiskTelemetryId = false;
proxyOptions: DevtoolsProxyOptions = {
Expand Down Expand Up @@ -1088,56 +1088,54 @@ export class CliRepl implements MongoshIOProvider {
* Close all open resources held by this REPL instance.
*/
async close(): Promise<void> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙂

markTime(TimingCategories.REPLInstantiation, 'start closing');
if (this.closing) {
return;
}
this.agent?.destroy();
if (!this.output.destroyed) {
// Wait for output to be fully flushed before exiting.
if (this.output.writableEnded) {
// .end() has been called but not finished; 'close' will be emitted in that case.
// (This should not typically happen in the context of mongosh, but there's also
// no reason not to handle this case properly.)
try {
await once(this.output, 'close');
} catch {
/* ignore */
return (this.closingPromise ??= (async () => {
markTime(TimingCategories.REPLInstantiation, 'start closing');
this.agent?.destroy();
if (!this.output.destroyed) {
// Wait for output to be fully flushed before exiting.
if (this.output.writableEnded) {
// .end() has been called but not finished; 'close' will be emitted in that case.
// (This should not typically happen in the context of mongosh, but there's also
// no reason not to handle this case properly.)
try {
await once(this.output, 'close');
} catch {
/* ignore */
}
} else {
// .end() has not been called; write an empty chunk and wait for it to be fully written.
await new Promise((resolve) => this.output.write('', resolve));
}
} else {
// .end() has not been called; write an empty chunk and wait for it to be fully written.
await new Promise((resolve) => this.output.write('', resolve));
}
}
markTime(TimingCategories.REPLInstantiation, 'output flushed');
this.closing = true;
const analytics = this.toggleableAnalytics;
let flushError: string | null = null;
let flushDuration: number | null = null;
if (analytics) {
const flushStart = Date.now();
try {
await analytics.flush();
markTime(TimingCategories.Telemetry, 'flushed analytics');
} catch (err: any) {
flushError = err.message;
} finally {
flushDuration = Date.now() - flushStart;
}
}
this.logWriter?.info(
'MONGOSH',
mongoLogId(1_000_000_045),
'analytics',
'Flushed outstanding data',
{
flushError,
flushDuration,
markTime(TimingCategories.REPLInstantiation, 'output flushed');
const analytics = this.toggleableAnalytics;
let flushError: string | null = null;
let flushDuration: number | null = null;
if (analytics) {
const flushStart = Date.now();
try {
await analytics.flush();
markTime(TimingCategories.Telemetry, 'flushed analytics');
} catch (err: any) {
flushError = err.message;
} finally {
flushDuration = Date.now() - flushStart;
}
}
);
await this.logWriter?.flush();
markTime(TimingCategories.Logging, 'flushed log writer');
this.bus.emit('mongosh:closed');
this.logWriter?.info(
'MONGOSH',
mongoLogId(1_000_000_045),
'analytics',
'Flushed outstanding data',
{
flushError,
flushDuration,
}
);
await this.logWriter?.flush();
markTime(TimingCategories.Logging, 'flushed log writer');
this.bus.emit('mongosh:closed');
})());
}

/**
Expand Down
Loading