Skip to content

feat(core): add config to skip waiting for browser close #120

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

Closed
doteric wants to merge 1 commit into from
Closed

feat(core): add config to skip waiting for browser close #120

Show file tree
Hide file tree
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
3 changes: 3 additions & 0 deletions packages/core/src/lib/Config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,7 @@ export class MemLabConfig {
noReCluster: boolean;
maxSamplesForClustering: number;
filterTraceByName: Nullable<string>;
skipBrowserCloseWait: boolean;

constructor(options: ConfigOption = {}) {
// init properties, they can be configured manually
Expand Down Expand Up @@ -370,6 +371,8 @@ export class MemLabConfig {
// if specified via CLI options, this will filter leak traces by
// node and edge names in the leak trace
this.filterTraceByName = null;
// if true, memlab will not wait for the browser to close successfully
this.skipBrowserCloseWait = false;
}

// initialize configurable parameters
Expand Down
2 changes: 2 additions & 0 deletions packages/core/src/lib/Utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1325,6 +1325,8 @@ async function closePuppeteer(
if (config.isLocalPuppeteer && !options.warmup) {
await Promise.all(pages.map(page => page.close()));
await browser.disconnect();
} else if (config.skipBrowserCloseWait === true) {
browser.close();
Copy link
Member

@JacksonGL JacksonGL May 22, 2024

Choose a reason for hiding this comment

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

In this case, skipping waiting for the promising to resolve may allow memlab to proceed, but after memory leak detection is done, the JS runtime would still not able to terminate since it would wait for all promises to resolve. Do you force terminate the node.js runtime somehow in the end?

Copy link
Author

@doteric doteric May 24, 2024

Choose a reason for hiding this comment

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

Thanks for the question!
We use Memlab inside AWS Lambdas where we do not have to wait for the runtime to finish. It keeps running and in the meantime if the browser finally closes then good, if not then AWS Lambda will terminate the runtime anyway. Ideally, of course, would be the browser.close() to not hang and just close the browser properly, but it's not so straightforward why the closing hangs, it would require deeper investigation.

In some cases iterating through all the opened pages and closing them before closing the browser worked, but in case of Memlab this does not seem to work correctly as the browser instance seems to return no opened pages.
Sparticuz/chromium#85 (comment)

Please let me know if I explained this fine or would you want further clarification. Happy to discuss further if needed

Copy link
Member

@JacksonGL JacksonGL May 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@doteric That makes sense. Thanks for the context. Please check and accept the CLA agreement so I can import and integrate your change.

https://github.com/facebook/memlab/blob/main/CONTRIBUTING.md#contributor-license-agreement-cla

} else {
await browser.close();
}
Expand Down